Merge pull request #31 from credbull/audit-fix

preliminary audit fixes

packages/contracts/src/base/CredbullBaseVault.sol

@@ -128,25 +128,39 @@ abstract contract CredbullBaseVault is ICredbull, ERC4626, Pausable {
         return decimal;
     }
 
-    /// @notice The share token should be soul bound. Should be transferable only to vault to receive assets back
+    /// @notice The share token should not be transferable.
     function transfer(address to, uint256 value) public override(ERC20, IERC20) returns (bool) {
-        if (to != address(this)) revert CredbullVault__TransferOutsideEcosystem();
-        address owner = _msgSender();
-        _transfer(owner, to, value);
+        revert CredbullVault__TransferOutsideEcosystem();
         return true;
     }
 
-    /// @notice The share token should be soul bound. Should be transferable only to vault to receive assets back
+    /// @notice The share token should not be transferable.
     function transferFrom(address from, address to, uint256 value) public override(ERC20, IERC20) returns (bool) {
-        if (to != address(this)) revert CredbullVault__TransferOutsideEcosystem();
-        address spender = _msgSender();
-        _spendAllowance(from, spender, value);
-        _transfer(from, to, value);
+        revert CredbullVault__TransferOutsideEcosystem();
         return true;
     }
 
     /// @notice Decimal value of share token is same as asset token
     function decimals() public view override returns (uint8) {
         return VAULT_DECIMALS;
     }
+
+    /// @notice Revert any ETH transfer to contract
+    receive() external payable {
+        revert();
+    }
+
+    /// @notice Revert any ETH transfer to contract
+    fallback() external payable {
+        revert();
+    }
+
+    /// @notice Withdraw any ERC20 tokens sent directly to contract.
+    /// This should be implemented by the inherited contract and should be callable only by the admin.
+    function _withdrawERC20(address[] calldata _tokens, address _to) internal {
+        for (uint256 i = 0; i < _tokens.length; i++) {
+            uint256 balance = IERC20(_tokens[i]).balanceOf(address(this));
+            SafeERC20.safeTransfer(IERC20(_tokens[i]), _to, balance);
+        }
+    }
 }

packages/contracts/src/vaults/FixedYieldVault.sol

@@ -92,4 +92,8 @@ contract FixedYieldVault is MaturityVault, WhitelistPlugIn, WindowPlugIn, MaxCap
     function unpauseVault() public onlyRole(DEFAULT_ADMIN_ROLE) {
         _unpause();
     }
+
+    function withdrawERC20(address[] calldata _tokens) public onlyRole(DEFAULT_ADMIN_ROLE) {
+        _withdrawERC20(_tokens, msg.sender);
+    }
 }

packages/contracts/src/vaults/UpsideVault.sol

@@ -20,7 +20,7 @@ contract UpsideVault is FixedYieldVault {
     /// @notice Percentage of collateral (100_00) is 100%
     uint256 public collateralPercentage;
 
-    mapping(address account => uint256) private _balances;
+    mapping(address account => uint256) private _collateralBalance;
 
     /// @notice Total collateral deposited
     // uint256 public totalCollateralDeposited;
@@ -53,6 +53,7 @@ contract UpsideVault is FixedYieldVault {
         internal
         override
         depositModifier(caller, receiver, assets, shares)
+        whenNotPaused
     {
         (, uint256 reminder) = assets.tryMod(10 ** VAULT_DECIMALS);
         if (reminder > 0) {
@@ -61,14 +62,14 @@ contract UpsideVault is FixedYieldVault {
 
         uint256 collateral = getCollateralAmount(assets);
 
-        _balances[receiver] += collateral;
+        _collateralBalance[receiver] += collateral;
         totalAssetDeposited += assets;
 
         if (totalAssetDeposited > maxCap) {
             revert CredbullVault__MaxCapReached();
         }
 
-        SafeERC20.safeTransferFrom(token, _msgSender(), address(this), collateral);
+        SafeERC20.safeTransferFrom(token, caller, address(this), collateral);
         SafeERC20.safeTransferFrom(IERC20(asset()), caller, CUSTODIAN, assets);
 
         _mint(receiver, shares);
@@ -81,14 +82,15 @@ contract UpsideVault is FixedYieldVault {
         internal
         override
         withdrawModifier(caller, receiver, owner, assets, shares)
+        whenNotPaused
     {
         if (caller != owner) {
             _spendAllowance(owner, caller, shares);
         }
 
         uint256 collateral = calculateTokenRedemption(shares, owner);
 
-        _balances[owner] -= collateral;
+        _collateralBalance[owner] -= collateral;
         totalAssetDeposited -= assets;
 
         SafeERC20.safeTransfer(token, receiver, collateral);
@@ -112,7 +114,7 @@ contract UpsideVault is FixedYieldVault {
         }
 
         uint256 sharePercent = shares.mulDiv(PRECISION, balanceOf(account));
-        return _balances[account].mulDiv(sharePercent, PRECISION);
+        return _collateralBalance[account].mulDiv(sharePercent, PRECISION);
     }
 
     /// @notice - Update the twap value

packages/contracts/test/factories/CredbullFixedYieldVaultFactory.t.sol

@@ -37,7 +37,7 @@ contract CredbullFixedYieldVaultFactoryTest is Test {
         factory.allowCustodian(params.custodian);
 
         vm.prank(config.factoryParams.operator);
-        CredbullFixedYieldVault vault = CredbullFixedYieldVault(factory.createVault(params, OPTIONS));
+        CredbullFixedYieldVault vault = CredbullFixedYieldVault(payable(factory.createVault(params, OPTIONS)));
 
         assertEq(vault.asset(), address(params.asset));
         assertEq(vault.name(), params.shareName);
@@ -130,6 +130,6 @@ contract CredbullFixedYieldVaultFactoryTest is Test {
         factory.allowCustodian(params.custodian);
 
         vm.prank(config.factoryParams.operator);
-        vault = CredbullFixedYieldVault(factory.createVault(params, OPTIONS));
+        vault = CredbullFixedYieldVault(payable(factory.createVault(params, OPTIONS)));
     }
 }

packages/contracts/test/factories/CredbullUpsideVaultFactory.sol

@@ -36,7 +36,7 @@ contract CredbullVaultWithUpsideFactoryTest is Test {
 
         vm.prank(config.factoryParams.operator);
         CredbullFixedYieldVaultWithUpside vault = CredbullFixedYieldVaultWithUpside(
-            factory.createVault(params, config.factoryParams.collateralPercentage, OPTIONS)
+            payable(factory.createVault(params, config.factoryParams.collateralPercentage, OPTIONS))
         );
 
         assertEq(vault.asset(), address(params.asset));

packages/contracts/test/vaults/CredbullFixedYieldVaultTest.t.sol

@@ -269,6 +269,21 @@ contract CredbullFixedYieldVaultTest is Test {
         vault.updateWindow(100, 200, 300, 400);
     }
 
+    function test__FixedYieldVault__ShouldAllowAdminToWithdrawERC20Tokens() public {
+        MockStablecoin token = MockStablecoin(address(vaultParams.asset));
+        vm.prank(alice);
+        token.transfer(address(vault), 100 * precision);
+
+        assertEq(token.balanceOf(address(vault)), 100 * precision);
+
+        vm.prank(vaultParams.owner);
+        address[] memory addresses = new address[](1);
+        addresses[0] = address(vaultParams.asset);
+        vault.withdrawERC20(addresses);
+
+        assertEq(token.balanceOf(address(vault)), 0);
+    }
+
     function deposit(address user, uint256 assets, bool warp) internal returns (uint256 shares) {
         // first, approve the deposit
         vm.startPrank(user);