code-423n4 / 2023-12-autonolas-findings

3 stars 3 forks source link

Lack of slippage control when querying price from UniswapV2 #61

Closed c4-bot-6 closed 10 months ago

c4-bot-6 commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/tokenomics/contracts/GenericBondCalculator.sol#L70-L92 https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/tokenomics/contracts/Depository.sol#L491-L493

Vulnerability details

Bug Description

The getCurrentPriceLP method of the GenericBondCalculator contract utilizes the current price from Uniswap V2 to calculate the current price of a specific token. However, the method lacks any slippage control when querying the price from Uniswap V2. This means that the returned price could be easily manipulated through the use of flash loans or other price manipulation techniques.

Impact

The absence of slippage controls makes the system vulnerable to price manipulation attacks. For instance, a malicious user could leverage a flash loan to execute substantial transactions on the associated Uniswap V2 pool, temporarily influencing the token price.

The impact of this bug could extend to various aspects of the system, including any contracts or protocols relying on accurate prices to make financial decisions. Moreover, it might compromise the integrity and correctness of financial operations based on these prices.

Proof of Concept

as we can see from the code:

    function getCurrentPriceLP(address token) external view returns (uint256 priceLP)
    {
        IUniswapV2Pair pair = IUniswapV2Pair(token);
        uint256 totalSupply = pair.totalSupply();
        if (totalSupply > 0) {
            address token0 = pair.token0();
            address token1 = pair.token1();
            uint256 reserve0;
            uint256 reserve1;
            // requires low gas
            (reserve0, reserve1, ) = pair.getReserves();
            // token0 != olas && token1 != olas, this should never happen
            if (token0 == olas || token1 == olas) {
                // If OLAS is in token0, assign its reserve to reserve1, otherwise the reserve1 is already correct
                if (token0 == olas) {
                    reserve1 = reserve0;
                }
                // Calculate the LP price based on reserves and totalSupply ratio multiplied by 1e18
                // Inspired by: https://github.com/curvefi/curve-contract/blob/master/contracts/pool-templates/base/SwapTemplateBase.vy#L262
                priceLP = (reserve1 * 1e18) / totalSupply;
            }
        }
    }

the price is calculated directly using the OLAS reserve on UniswapV2,without considering the slippage that maybe in that moment the market passed,inflating the price in the direction wanted by the attacker could be used to stole funds from others protocol relying on this method return,which can be harmful to the end user.

Tools Used

manual review

Recommended Mitigation Steps

the model taken as an example to calculate the price is correct,but not safe,in fact as said before could be easily manipulated,so imo the protocol should adapt like a new solution to price the token,for example by using the "Fair Uniswap's LP Token Pricing" from alpha finance.

Assessed type

Invalid Validation

c4-pre-sort commented 10 months ago

alex-ppg marked the issue as duplicate of #345

c4-pre-sort commented 10 months ago

alex-ppg marked the issue as insufficient quality report

c4-judge commented 9 months ago

dmvt marked the issue as unsatisfactory: Invalid