Asset manager can completely drain all user funds from Vault

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0xb53a7b241e8ca39cde2b0c141b81263cb0dd969db51b3814376eaa38cbafdc9e Severity: high

Description:

Impact

• Total loss of all user funds held in the vault
• Total loss of all approved funds by the users to the vault

Description

The root of the problem is that the AM can execute any arbitrary call via claimRewardTokens() as long as it increases the balance of _tokenToBeClaimed and tokenBalancesInVault.

Rebalancing.sol - claimRewardTokens()

  function claimRewardTokens(
    address _tokenToBeClaimed,
    address _target,
    bytes memory _claimCalldata
  ) external onlyAssetManager {
    // Retrieve the list of all tokens in the portfolio and their balances before the claim operation
    address[] memory tokens = portfolio.getTokens();
    uint256[] memory tokenBalancesInVaultBefore = getTokenBalancesOf(
      tokens,
      _vault
    );

    uint256 rewardTokenBalanceBefore = _getTokenBalanceOf( 
      _tokenToBeClaimed,
      _vault
    );

    // Execute the claim operation using the provided calldata on the target contract
    portfolio.claimRewardTokens(_target, _claimCalldata);

    uint256[] memory tokenBalancesInVaultAfter = getTokenBalancesOf(
      tokens,
      _vault
    );

    // Fetch the new balance of the reward token in the vault after the claim operation
    uint256 rewardTokenBalanceAfter = _getTokenBalanceOf(
      _tokenToBeClaimed,
      _vault
    );

    // Ensure the reward token balance has increased, otherwise revert the transaction
    if (rewardTokenBalanceAfter <= rewardTokenBalanceBefore)
      revert ErrorLibrary.ClaimFailed();

    // Check that no other token balances have decreased, ensuring the integrity of the vault's assets
    uint256 tokensLength = tokens.length;
    for (uint256 i; i < tokensLength; i++) {
      if (tokenBalancesInVaultAfter[i] < tokenBalancesInVaultBefore[i])
        revert ErrorLibrary.ClaimFailed();
    }
  }

This opens up an attack path where the asset manager can overcome these restrictions with adding a custom token created by himself to the vault which will return a higher balance on subsequent balanceOf calls. Note that the _getTokenBalanceOf() calls used in the function call .balanceOf() of the tokens added which can return anything.

VaultManager.sol - claimRewardTokens()

  function claimRewardTokens(
    address _target,
    bytes memory _claimCalldata
  ) external onlyRebalancerContract {
    // Execute the transfer through the safe module and check for success
    (, bytes memory data) = IVelvetSafeModule(safeModule).executeWallet( 
      _target,
      _claimCalldata
    );

    // Ensure the transfer was successful; revert if not
    if (!(data.length == 0 || abi.decode(data, (bool)))) {
      revert ErrorLibrary.ClaimFailed();
    }
  }

Proof of Concept

Transfer out all vault holdings

1. Users deposit to vault -> vault gathers good amount of liquidity
2. AM creates a custom token MaliciousERC20 that returns higher uint256 when it's called every next time with balanceOf(). This way both the tokenBalancesInVaultAfter and rewardTokenBalanceAfter will be higher then the before variants.

example of custom balanceOf() function:

contract MaliciousERC20 {
    ...
    uint256 counter;
    function balanceOf(address account) public view virtual returns (uint256) {
        return counter += uint256(type(uint64).max);
    }
    ...
}

3. AM creates very small LP before to execute the handler swap later in _updateWeights() that will be called in updateTokens()
4. AM adds the MaliciousERC20 token via calling updateTokens()
5. AM can now bypass the safeguard checks with every external call
6. AM calls ERC20.transfer(assetManager, ERC20.balanceOf(portfolio)) with every deposited token in the vault via Rebalancing.claimRewardTokens() with _tokenToBeClaimed as MaliciousERC20, _target as the held ERC20 and _claimCalldata as the encoded ERC20.transfer()
7. Users lose all their deposited funds, AM walks away with the total holdings of the vault as profit

Steal approved funds before deposit

1. Same setup as first scenario steps 1-4
2. User intends to deposit to the vault so first he needs to approve his token via USDC.approve(depositAmount, portfolio)
3. After the approval but before the deposit call of user: asset manager can call .transferFrom(user, portfolio) to transfer the funds to the portfolio without the user actually depositing
4. Asset manager now can simply transfer out the user funds with USDC.transfer(assetManager, depositAmount) call through claimRewardTokens()
5. Users lose all their approved funds, note that users may lose up to the total amount of funds held by their wallet if they set up an infinite approval to the contract

Approve out all funds

1. Same setup as first scenario steps 1-4
2. AM approves all tokens out before deposits with an approval call through claimRewardTokens(): ERC20.approve(assetManager, type(uint256).max).
3. When enough liquidity is gathered in Vault the AM can call ERC20.safeTransferFrom(portfolio, assetManager, amount) directly on the respective ERC20s with all of the tokens
4. Users lose all their deposited funds, AM walks away with the total holdings of the vault as profit

Recommendation

Consider to explicitly check if the _claimCallData is intending to call the function selector of:

• transfer() - 0xa9059cbb
• transferFrom() - 0x095ea7b3
• approve() - 0x23b872dd

and revert the call as these would attempt to steal assets from the Vault. Consider to check if _target is one of the tokens of the vault and revert the call in claimRewardTokens().

[langnavina97]

We are aware of the risk that asset managers can send calldata to the vault to execute any transaction from there. However, there are checks in place to ensure that the token balance of the reward token increases and the portfolio token balances don't increase.

Adding a check to verify that the calldata doesn't include those functions would not be effective. An attacker could write a contract containing the approve functions (delegatecall) to approve all tokens while simultaneously claiming the rewards, ensuring the transaction doesn't fail during the verification process.

Asset managers can also rebalance the portfolio to any tokens if there is no whitelisting, providing other ways to drain the funds. However, the asset manager should act in the best interest of the holders to manage and earn performance fees. These potential attacks are restricted to asset managers only.

@0xfuje

[chainNue]

I'm not following the whole issue, but as balanceOf is a view function, thus the counter will be reverted back after the call, so it will not be increased. The same initial assumption on #68

[0xfuje]

I'm not following the whole issue, but as balanceOf is a view function, thus the counter will be reverted back after the call, so it will not be increased. The same initial assumption on https://github.com/hats-finance/Velvet-Capital-0x0bb0c08fd9eeaf190064f4c66f11d18182961f77/issues/68

It was just an example, but true, however it's not hard to bypass this, since you can make any state changing call to raise the balance of the token in claimRewardsToken() before the transfer out or approval calls