c4-bot-6 commented 9 months ago

Lines of code

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

Vulnerability details

Description

The function Pools::addLiquidity() allows users to deposit liquidity/collateral to the pools such that the final ratio of the reserves are maintained that has already been established in the pool.

For a given amount for token 0, it determined how much of the second token to deposit based on the following:

uint256 proportionalB = (maxAmount0 * reserve1) / reserve0;

But the above can be exploited for rounding issue, such that the final proportionalB becomes 0. This can be achieved if the value provided for maxAmount0 is such that it makes (maxAmount0 * reserve1) / reserve0 less than 1. If such an amount for maxAmount0 is provided which makes proportionalB as 0, then the user can get away by just providing liquidity/collateral as one token, and skip the other token completely. This will also cause the ratio of the pool reserves to change.

Impact

Because of this rounding issue, a user can make proportionalB as 0. So the user will not have to provide any tokens for reserve1, and only reserve0 will be enough. So the user can bypass depositing of both the tokens.

Also, because the user is now just providing one token, the ratio of the pool reserves will be changed.

Proof of concept

Use the below test in the file: src/pools/tests/Pools.t.sol

function testNeoCraoDepositOnlyReserve0Tokens() public {
    // Setup Tokens

    TestERC20[] memory testTokens = new TestERC20[](2);

    vm.startPrank(DEPLOYER);
    for (uint256 i = 0; i < 2; i++) {
        testTokens[i] = new TestERC20("TEST", 18);
    }
    (, bool flipped) = PoolUtils._poolIDAndFlipped(testTokens[0], testTokens[1]);
    if (flipped) {
        (testTokens[1], testTokens[0]) = (testTokens[0], testTokens[1]);
    }
    for (uint256 i = 0; i < 2; i++) {
        testTokens[i].approve(address(pools), type(uint256).max);
        testTokens[i].approve(
            address(collateralAndLiquidity),
            type(uint256).max
        );

        testTokens[i].transfer(address(this), 100000 ether);
        testTokens[i].transfer(address(dao), 100000 ether);
        testTokens[i].transfer(address(collateralAndLiquidity), 100000 ether);
    }
    vm.stopPrank();

    vm.prank(address(dao));
    poolsConfig.whitelistPool(pools, testTokens[0], testTokens[1]);

    vm.prank(DEPLOYER);
    collateralAndLiquidity.depositLiquidityAndIncreaseShare(
        testTokens[0],
        testTokens[1],
        5000 ether,
        10 ether,
        0,
        block.timestamp,
        false
    );

    for (uint256 i = 0; i < 2; i++) {
        testTokens[i].approve(address(pools), type(uint256).max);
        testTokens[i].approve(
            address(collateralAndLiquidity),
            type(uint256).max
        );
    }

    vm.startPrank(address(collateralAndLiquidity));
    testTokens[0].approve(address(pools), type(uint256).max);
    testTokens[1].approve(address(pools), type(uint256).max);

    // POC

    vm.startPrank(address(collateralAndLiquidity));

    (uint256 reserves0Initial, uint256 reserves1Initial) = pools.getPoolReserves(
        testTokens[0],
        testTokens[1]
    );

    uint256 totalShares = collateralAndLiquidity.totalShares(
        PoolUtils._poolID(testTokens[0], testTokens[1])
    );

    (
        uint256 addedAmountA,
        uint256 addedAmountB,
        uint256 addedLiquidity
    ) = pools.addLiquidity(
        testTokens[0],
        testTokens[1],
        (reserves0Initial/reserves1Initial) - 1,
        1 ether,
        0,
        totalShares
    );

    (uint256 reserves0Final, uint256 reserves1Final) = pools.getPoolReserves(
        testTokens[0],
        testTokens[1]
    );
    assertEq(addedAmountB, 0);
    assertEq(reserves1Initial, reserves1Final);
}

Severity Justification

This is Medium severity, based on the Code4rena Severity Categorization: https://docs.code4rena.com/awarding/judging-criteria/severity-categorization

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Tools Used

Manual Review

Recommended Mitigation Steps

In the function Pools::addLiquidity() it calls Pools::_addLiquidity(), which has the logic to calculate proportionalB. A check like the below can help avoid such issues:

require(proportionalB > PoolUtils.DUST, "Not enough liquidity/collateral being added");

Assessed type

Other

c4-judge commented 9 months ago

Picodes marked the issue as primary issue

c4-sponsor commented 8 months ago

othernet-global (sponsor) confirmed

othernet-global commented 8 months ago

addLIquidity now requires addedAmount0 and addedAmount1 to be greater than DUST.

Fixed in: https://github.com/othernet-global/salty-io/commit/0bb763cc67e6a30a97d8b157f7e5954692b3dd68

Picodes commented 8 months ago

This report doesn't show how this could lead to economic harm. The rounding error is always happening, what is the issue with the fact that potentially one of the amounts is 0?

c4-judge commented 8 months ago

Picodes marked the issue as unsatisfactory: Insufficient proof

SolSaver commented 8 months ago

Hello @Picodes!

Thank you for looking into this issue. I am responding here as my issue #775 was duped with this.

This report doesn't show how this could lead to economic harm. The rounding error is always happening, what is the issue with the fact that potentially one of the amounts is 0?

In the proof of concept of my issue as well as this issue, it has been demonstrated that a user can manipulate their request such that only one of the reserves get increased. This allows the user to provide funds for only one token for the token pair pool. In the regular flow, a user is required to provide both tokens proportional to the existing ratio of the tokens in the pool.

For example, if the pool has ETH:BTC ratio as 2:1, then a user has to provide 1 BTC for every 2 ETH so that the ratio is maintained. However, this issue highlights that:

A user can get away by providing only 1 of the tokens. Whereas all the other users in a normal flow will have to provide amounts for both the tokens.
The ratio of the pool will not be the same after the malicious user provides only 1 token. Because of this imbalance, other users will now be affected by the changed ratio, which is something that should not happen.

Also, the sponsor has confirmed the issue and has also posted a fix, which further supports the argument of this being a valid issue.

neocrao commented 8 months ago

Hello @Picodes I appreciate the review and the comments. I think @SolSaver covered most of what I wanted to say.

This report doesn't show how this could lead to economic harm.

Thanks for the feedback. Such concrete feedback will help me in writing future reports well. However, I hope that the comments on this issue provides new data towards the economic harm.

The rounding error is always happening

This is not true. Rounding error only happens when the user provides a specific ratio for the amounts, which makes proportionalB as 0, thereby making the amount required for token B as 0 as well.

what is the issue with the fact that potentially one of the amounts is 0?

(I'll re-use the example from the previous comment to make it easy) Lets say there is a pool with ETH and BTC with the token ratio as 2:1. When calling addLiquidity() function, a user needs to provide amounts for both ETH and BTC so that the proportions match the pool ratio. This is mentioned as comments in the function as well "// Add liquidity to the pool proportional to the current existing token reserves in the pool."

Example 1: If a user sends 20 ETH and 5 BTC, then the pool will accept 10 ETH, and 5 BTC, and send the remaining 10 ETH back. So overall a user deposited 10 ETH and 5 BTC. Example 2: Similarly, if a user sends 5 ETH and 20 BTC, then the pool will accept 5 ETH, and 2.5 BTC, and send the remaining 17.5 BTC back. So overall a user deposited 5 ETH and 2.5 BTC.

This issue is trying to convey that a user can send in a request that allows them to deposit only 1 token because of rounding issue. In the above examples, a user can get away by just depositing ETH, and not provide anything for BTC. So this makes it a cheaper transaction for the user.

The other economic aspect is that the pool ratios get imbalanced. It is required for users to deposit the tokens in the same proportion as the pool token ratio, so that the overall token ratio of the pool remains the same after user's deposits. But if a user can deposit only 1 token, then this imbalances the pool token ratio.

Picodes commented 8 months ago

This issue is actually a duplicate of #941 and #111. Other comments regarding this can be found on #941.

Picodes commented 8 months ago

@SolSaver @neocrao I am sorry but I don't see how you have proven anything here in your comments. I have understood that the user is only depositing one token but this isn't an issue in itself unless it is actionable by an attacker. You discuss 2 scenarios. First a user depositing only 1 token but you don't show how this is profitable for him. Then the fact that the ratio is changing but you don't discuss the numbers.

Imagine a pool where the ratio is 1000:1. Depositing anything below 999 token A will lead to 0 token B deposited. Ok. So the ratio is moving from X, Y to X + 1000, Y. In an extreme case where the initial reserves is 1000:1 it does 1000:1 -> 2000:1. Now imagine someone depositing 9999:9. The ratio is moving from X, Y to X + 9999, Y + 9. In the extreme case it does 1000:1 -> 11000:10 = 1100:1.

All this is to show that because of the rounding approximations, the ratio is always moving a bit when you add liquidity whether you add 1 or 2 tokens.

neocrao commented 8 months ago

Hello @Picodes. Thank you for the comments.

Let me try again with some more numbers.

First a user depositing only 1 token but you don't show how this is profitable for him.

Lets say a pool has 2000 ETH and 10 BTC, that is a ratio of ETH:BTC as 200:1. Reserve0 is ETH, and Reserve 1 is BTC.

Normal User Depositing: Now, a normal user would send, lets say, 1000 ETH, and 100 BTC.

Based on this formula uint256 proportionalB = (maxAmount0 * reserve1) / reserve0;, lets calculate how much of BTC should the pool add for a given amount of ETH:

uint256 proportionalB = (maxAmount0 * reserve1) / reserve0;
uint256 proportionalB = (1000 * 10) / 2000;
uint256 proportionalB = 5;

So the pool will take in 1000 ETH, and 5 BTC from user, and return the remaining 95 BTC back to the user.

The pool ratio updates to:

(Old ETH Reserve amount + New ETH) : (Old BTC Reserve amount + New BTC amount)
2000 + 1000 : 10 + 5
3000 : 15
200 : 1

As you can see, the ratio remains the same as 200:1, and the pool reserves went from (2000 ETH, 10 BTC) to (3000 ETH, 15 BTC). So the user had to deposit (1000 ETH, 5 BTC) overall.

Attacker User Depositing: Now, an attacker attempts to deposit, and the attacker sends, lets say, 199 ETH, and 100 BTC.

Based on this formula uint256 proportionalB = (maxAmount0 * reserve1) / reserve0;, lets calculate how much of BTC should the pool add for a given amount of ETH:

uint256 proportionalB = (maxAmount0 * reserve1) / reserve0;
uint256 proportionalB = (199 * 10) / 2000;
uint256 proportionalB = 1990 / 2000;
uint256 proportionalB = 0;

So the pool will take in 199 ETH, and 0 BTC from attacker, and return all the 100 BTC back to the attacker.

The pool ratio updates to:

(Old ETH Reserve amount + New ETH) : (Old BTC Reserve amount + New BTC amount)
2000 + 199 : 10 + 0
2199 : 10
219 : 1 (Rounding down)

As you can see, the ratio changes from 200:1 to 219:1, and the pool reserves went from (2000 ETH, 10 BTC) to (2199 ETH, 10 BTC). So the attacker had to deposit (199 ETH, 0 BTC) overall.

This becomes a cheaper deposit for the attacker as compared to the normal user, as the attacker did not have to deposit any BTC at all.

Then the fact that the ratio is changing but you don't discuss the numbers.

I am hoping that the above explanation covers the numbers of the ratios changing. In the attacker flow the ratios changed from 200:1 to 219:1. So the next normal deposit will require more ETH for every BTC.

All this is to show that because of the rounding approximations, the ratio is always moving a bit when you add liquidity whether you add 1 or 2 tokens.

This is the issue. The protocol expects the user deposits to not alter the ratio, and hence a user needs to deposit both the tokens. If they deposit both the tokens, then the ratios cannot be altered. However, if the user is able to deposit only 1 token because of rounding issues, then they are able to alter the ratios, and hence it breaks the assumptions of the protocol. Such a user also gets financially benefited from just depositing 1 token as they don't have to provide any second token.

If this issue is still not clear, I will like to request a comment/engagement from the sponsor as the sponsor seems to have confirmed the issue and even posted a fix for it. Maybe the sponsor can provide a better explanation of what the protocol expects.

Thank you again for reviewing the issue and the comments thoroughly!

Picodes commented 8 months ago

"Such a user also gets financially benefited from just depositing 1 token as they don't have to provide any second token." -> this doesn't really make sense. Depositing 1 token is not inherently better than depositing 2 tokens. It all depends on the overall value of what you are depositing.

Picodes commented 8 months ago

@neocrao again I'll repeat myself but a PoC implying (2000 ETH, 10 BTC) isn't proving anything. ETH has 18 decimals, WBTC 8 so we are playing with dust amounts where rounding errors are very common but the amounts at stake are tiny so it has no impact.

Picodes commented 8 months ago

" The protocol expects the user deposits to not alter the ratio, and hence a user needs to deposit both the tokens. " -> That isn't true. The protocol expects that a user deposit doesn't alter the ratio too much and that the change in ratio can never create an exploitable opportunity.

The fact that a user deposit alters the ratio is relatively obvious here as we are working with integers.

neocrao commented 8 months ago

"Such a user also gets financially benefited from just depositing 1 token as they don't have to provide any second token." -> this doesn't really make sense. Depositing 1 token is not inherently better than depositing 2 tokens.

In the examples I provided earlier: For the normal user, they had to deposit (1000 ETH, 5 BTC) overall, which is:

Assuming 1 ETH = 2000 USD, and 1 BTC = 50,000 USD
= 1000 ETH + 5 BTC
= 1000 * 2000 + 5 * 50000
= 2,000,000 + 250,000
= 2,250,000 USD

Whereas, for the attacker user, they had to deposit (199 ETH, 0 BTC) overall, which is:

Assuming 1 ETH = 2000 USD, and 1 BTC = 50,000 USD
= 199 ETH + 0 BTC
= 199 * 2000 + 0 * 50000
= 398,000 + 0
= 398,000 USD

Lets say the attacker roughly does it 5 times to match 2000 ETH that the normal user deposited
Total deposited amount = 398,000 * 5 = 1,990,000 USD

We can see that the normal user had to deposit 2 tokens, and the attacker had to deposit 1 token, and that the overall USD amount between the two users varied in favor of the attacker.

It all depends on the overall value of what you are depositing.

Now, continuing the scenario from above, there was a difference in overall USD values between the two users. But furthermore lets see how the protocol determines the value as well.

The final likes of the Pools::_addLiquidity() function is:

        // Determine the amount of liquidity that will be given to the user to reflect their share of the total collateralAndLiquidity.
        // Use whichever added amount was larger to maintain better numeric resolution.
        // Rounded down in favor of the protocol.
        if (addedAmount0 > addedAmount1)
            addedLiquidity = (totalLiquidity * addedAmount0) / reserve0;
        else addedLiquidity = (totalLiquidity * addedAmount1) / reserve1;

So for the normal user as well as the attacker, this condition holds true addedAmount0 > addedAmount1, and hence the value of addedLiquidity will be the same for the user as well. This addedLiquidity is then used in Liquidity::_depositLiquidityAndIncreaseShare() as part of _increaseUserShare(msg.sender, poolID, addedLiquidity, true); to increase the user share.

In summary, the attacker gained the same amount of shared by spending less amount of USD value than what a normal user has to.

@neocrao again I'll repeat myself but a PoC implying (2000 ETH, 10 BTC) isn't proving anything. ETH has 18 decimals, WBTC 8 so we are playing with dust amounts where rounding errors are very common but the amounts at stake are tiny so it has no impact.

Thanks for reiterating. And its a great point. Maybe the example used isnt too intuitive because of the mismatching decimals. Do you think it will still be dust amounts if the tokens had the same decimals, such as DAI and WETH, both of which are 18 decimals?

" The protocol expects the user deposits to not alter the ratio, and hence a user needs to deposit both the tokens. " -> That isn't true. The protocol expects that a user deposit doesn't alter the ratio too much and that the change in ratio can never create an exploitable opportunity.

Lets say that the pool ratio is A:B, and a user deposits X and Y amount of tokens respectively. To maintain the same ratio as the pool, the amount of Y tokens in terms of X, A and B should be:

    A     X
=> --- = ---
    B     Y

        XB
=> Y = ---
        A

So the new pool ratio will be:

  A + X
= -----
  B + Y

  A + X
= ------
  B + XB
      --
       A

  A(A + X)
= -------
  AB + XB

  A (A+X)
= -------
  B(A+X)

   A
= ---
   B

You can see that the final ratio remains the same.

The fact that a user deposit alters the ratio is relatively obvious here as we are working with integers.

It is obvious, and thats why the protocol has implemented DUST checks to ensure that the amounts are atleast above than the DUST amounts.

I dont have any further arguments. My final request to you again will be to please run this by the sponsor too, as the sponsor has confirmed the issue, and posted a fix for it, and there should be a reason for that.

Picodes commented 8 months ago

I am not sure you understood my points.

Picodes commented 8 months ago

Your example is obviously invalid as it does not account for decimals. 1000 eth is worth 0. 1000 x 1e18 eth is worth something but then it won't round down to 0.

Picodes commented 8 months ago

Your reasoning about the ratio is also invalid as it doesn't account for the rounding error.

Picodes commented 8 months ago

The discussion is closed. I am not saying that there is no underlying issue but that none of the PoC provided so far were valid.

@othernet-global pinging you for review in case you want to take a look

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

A user can exploit rounding issue to deposit only one token to the pool #785

Lines of code

Vulnerability details

Description

Impact

Proof of concept

Severity Justification

Tools Used

Recommended Mitigation Steps

Assessed type