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

11 stars 6 forks source link

Breaking of protocol AMM invariant #1041

Closed c4-bot-7 closed 7 months ago

c4-bot-7 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L168-L188

Vulnerability details

Impact

When implementing an AMM, the token reserves are usually kept in a balanced ratio to avoid issues like price manipulation etc

Proof of Concept

When implementing an AMM, the token reserves are usually kept in a balanced ratio to avoid issues. We can see that in the removeliquidity() the reserves are checked after liquidity is removed to maintain that the reserves of each token are still above the Dust amount set by the protocol

                // Make sure that removing liquidity doesn't drive either of the reserves below DUST.
        // This is to ensure that ratios remain relatively constant even after a maximum withdrawal.
   require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

But this check made above is wrong as it validates just the reserve for a particular reserve. A malicious depositor or liquidity provider can be the first to deposit liquidity in a new deployed pool and supply lets say tokenA and tokenB and then after other LPs provides a certain amount of liquidity they withdraw all of their tokenB and the supposed amount of TokenA. This will cause a huge imbalance of tokens in the pool and can be used to manipulate price as the pool will be serving wrong prices due to this imbalance. In a situation like with the protocol that intend on using pool reserves as a source of price feed it can be generating wrong prices. The attacker can go ahead and do this for multiple other pools, thereby rrendering the pool reserves prices unusable. If should the chainlink price feeds have a downtime or an issue like i discussed in another finding then source for prices will be bricked

Tools Used

Manual Review

Recommended Mitigation Steps

require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve1 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

Assessed type

Other

c4-judge commented 8 months ago

Picodes marked the issue as primary issue

c4-judge commented 8 months ago

Picodes marked issue #647 as primary and marked this issue as a duplicate of 647

c4-judge commented 7 months ago

Picodes marked the issue as satisfactory

c4-judge commented 7 months ago

Picodes changed the severity to 2 (Med Risk)

c4-judge commented 7 months ago

Picodes marked the issue as selected for report

c4-judge commented 7 months ago

Picodes marked issue #784 as primary and marked this issue as a duplicate of 784