Zellic - Audit Reports

Category: Coding Mistakes

Return value checks for `transfer` and `transferFrom`

Medium Severity

Medium Impact

Low Likelihood

Description

In the BlackwingVault contract, the deposit() and withdraw() functions do not include checks on the return values of transfer and transferFrom. This parameter should be validated to ensure the success of the transfer.

Impact

If the transfer of assets fails to revert, the vault may mint tokens without actually receiving the asset. This would increase the totalSupply of the vault token but would not add to the balance obtained from getUndeployedAmount(), as called within the getTotalAssetAmount() function.

function deposit(IERC20 asset, uint amount) public {
  requireAssetRegistered(asset);

  PoolInfo memory pool = pools[asset];
  uint totalVaultTokenSupply = pool.vaultToken.totalSupply();
  uint vaultTokensToMint = 0;
  if (totalVaultTokenSupply == 0) {
    vaultTokensToMint = amount * INITIAL_LIQUIDITY_MULTIPLIER;
  } else {
    uint totalAssetBalance = getTotalAssetAmount(asset);
    // ...
    vaultTokensToMint = amount * totalVaultTokenSupply / totalAssetBalance;
  }
  require(vaultTokensToMint > 0, VAULT_TOKEN_GRANULARITY_ERR);
  asset.transferFrom(msg.sender, address(this), amount);
  pool.vaultToken.mint(msg.sender, vaultTokensToMint);
  // ...
}

function getTotalAssetAmount(IERC20 asset) internal view returns (uint) {
  return getUndeployedAmount(asset) + getDeployedAmount(asset);
}

function getDeployedAmount(IERC20 asset) internal view returns (uint) {
  return pools[asset].deployer.totalDeployedAmount(asset);
}

function getUndeployedAmount(IERC20 asset) internal view returns (uint) {
  return asset.balanceOf(address(this));
}

Recommendations

We recommend using a wrapper such as SafeERC20 so that noncompliant ERC-20 tokens that do not return a boolean are safely handled.

For example, like this:

// ...
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

contract BlackwingVault is Initializable, AccessControlUpgradeable {
+ using SafeERC20 for IERC20;
  // ...

  function deposit(IERC20 asset, uint amount) public {
    // ...
-   asset.transferFrom(msg.sender, address(this), amount);
+   asset.safeTransferFrom(msg.sender, address(this), amount);
    // ...
  }

  function withdraw(IERC20 asset, uint vaultTokensToBurn) public {
    // ...
-   asset.transfer(msg.sender, amountReturned);
+   asset.safeTransfer(msg.sender, amountReturned);
    // ...
  }

Remediation

This issue has been acknowledged by Ferum Labs, and a fix was implemented in commit ce6b2a83↗.

Return value checks for transfer and transferFrom

Description

Impact

Recommendations

Remediation

Return value checks for `transfer` and `transferFrom`