[NAZ-H5] Underlying Assets Stealing In All EIP4626 Implementations via share price manipulation

[code423n4]

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.

[c4-judge]

dmvt marked the issue as duplicate of #15

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as satisfactory