Popcorn contest - 0xNazgul's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol#L170

Vulnerability details

Impact

The claimRewards() function is used to claim rewards for a user in any amount of rewardTokens. Any ERC20 can be added meaning, any ERC777 token can be to since they are backwards-compatible.

Proof of Concept

This introduces a Reentracy vulnerability allowing anyone to get more accruedRewards draining the contract. Looking at The claimRewards() function:

function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
    for (uint8 i; i < _rewardTokens.length; i++) {
        uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];

        if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);

        EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];

        if (escrowInfo.escrowPercentage > 0) {
            _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);	//@audit if rewardToken were an erc777, it can reenter draining 
            emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
        } else {
            _rewardTokens[i].transfer(user, rewardAmount);			//@audit if rewardToken were an erc777, it can reenter draining 
            emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
        }

	accruedRewards[user][_rewardTokens[i]] = 0;
    }
}

Specifically line #L186 which is the point where the users accruedRewards is updated. Because it is done after both _lockToken() and transfer() Reentracy is possible with the use of ERC777.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider ensuring that no ERC777 tokens are able to be used as a reward token. Could also use a Reentrancy modifier and move line #L186 before like so:

function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
    for (uint8 i; i < _rewardTokens.length; i++) {
        uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];

        if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);

        EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];

        accruedRewards[user][_rewardTokens[i]] = 0; 		//@audit move accruedRewards here to prevent Reentrancy

        if (escrowInfo.escrowPercentage > 0) {
            _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);//@audit if rewardToken were an erc777, it can reenter draining 
            emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
        } else {
            _rewardTokens[i].transfer(user, rewardAmount);
            emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
        }
    }
}

Lines of code

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L17 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/beefy/BeefyAdapter.sol#L17 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L88 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L154 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol#L26 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L26

Vulnerability details

Impact

Vault tokens can be stolen from depositors in all EIP4626 Implementations vaults by manipulating the price of a share.

Proof of Concept

ERC4626 vaults are subject to a share price manipulation attack that allows an attacker to steal underlying tokens from other depositors (this is a known issue of ERC4626 implementation). Consider this scenario for Vault.sol:

1. Malaclypse is the first depositor of the vault;
2. Malaclypse deposits 1 wei of tokens;
3. In the deposit function (Vault.sol#L134), the amount of shares is calculated using the convertToShares() function:

function convertToShares(uint256 assets) public view returns (uint256) {
    uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

    return
        supply == 0
            ? assets
            : assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);
}

4. Since Malaclypse is the first depositor (totalSupply is 0), she gets 1 share (1 wei);
5. Malaclypse then sends 9999999999999999999 tokens (10e18 - 1) to the vault;
6. The price of 1 share is 10 tokens now: Malaclypse is the only depositor in the vault, she's holding 1 wei of shares, and the balance of the pool is 10 tokens;
7. Alice deposits 19 tokens and gets only 1 share due to the rounding in the convertToShares() function: 19e18 * 1 / 10e18 == 1;
8. Malaclypse redeems her share and gets a half of the deposited assets, 14.5 tokens (less the withdrawal fee);
9. Alice redeems her share and gets only 14.5 (less the withdrawal fee), instead of the 19 he deposited.

Tools Used

Manual Review

Recommended Mitigation Steps

1. In the deposit() functions, consider requiring a reasonably high minimal amount of assets during first deposit. The amount needs to be high enough to mint many shares to reduce the rounding error and low enough to be affordable to users.
2. On the first deposit, consider minting a fixed and high amount of shares, irrespective of the deposited amount.
3. Consider seeding the pools during deployment. This needs to be done in the deployment transactions to avoiding front-running attacks. The amount needs to be high enough to reduce the rounding error.
4. Consider sending first 1000 wei of shares to the zero address. This will significantly increase the cost of the attack by forcing an attacker to pay 1000 times of the share price they want to set. For a well-intended user, 1000 wei of shares is a negligible amount that won't diminish their share significantly.

Lines of code

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/VaultController.sol#L238

Vulnerability details

Impact

Across the VaultController.sol there are many external calls to the AdminProxy.sol via execute(). Looking at the execute() function in AdminProxy.sol:

function execute(address target, bytes calldata callData)
    external
    onlyOwner 
    returns (bool success, bytes memory returndata) 
{
    return target.call(callData);
}

As one can see it does a call to the target contract with the provided callData. Going back to the VaultController.sol the success of the call is check and reverts if unsuccessful. However, in one instance this check is missed and could cause issues.

Proof of Concept

Looking at that specific instance:

function __deployAdapter(
    DeploymentArgs memory adapterData,
    bytes memory baseAdapterData,
    IDeploymentController _deploymentController
  ) internal returns (address adapter) {
    (bool success, bytes memory returnData) = adminProxy.execute(
      address(_deploymentController),
      abi.encodeWithSelector(DEPLOY_SIG, ADAPTER, adapterData.id, _encodeAdapterData(adapterData, baseAdapterData))
    );
    if (!success) revert UnderlyingError(returnData);

    adapter = abi.decode(returnData, (address));

    adminProxy.execute(adapter, abi.encodeWithSelector(IAdapter.setPerformanceFee.selector, performanceFee));//@audit unchecked like the others are
}

It is clear that the last call to AdminProxy.sol's execute is not checked.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a check similar to how it is done in the rest of the contract.

Lines of code

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L525

Vulnerability details

Impact

Currently in Vault.sol there are four different fee types:

1. Deposit
2. Withdrawal
3. Management
4. Performance

There is a proper check to make sure that individually none of them are >= 1e18. However, they can total to more than 1e18 and cause unsuspecting users to pay more than they may want to.

Proof of Concept

Just taking the deposit and withdrawal fees into account say both of them are set to 5e17, totaling to 1e18. If a user were to than deposit and withdraw that would be 100%. Add in the other two fees and this situation gets even worse.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider making sure that the total of all four fee types to be less than 1e18 instead of individually.

Lines of code

https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L17 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/beefy/BeefyAdapter.sol#L17 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L88 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L154 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol#L26 https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol#L26

Vulnerability details

Impact

There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom(). Others are rebasing tokens that increase in value over time like Aave's aTokens (balanceOf changes over time).

Proof of Concept

Across a multiple different functions in all of the contracts will will store the entire amount but with fee-on-transfer tokens, fewer tokens will be transferred which leads to inconsistencies.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider checking actual balance of the contract or ensure that the protocol never uses rebasing or tokens with fee-on transfer.