search

code-423n4 / 2022-04-badger-citadel-findings

0 stars 1 forks source link

Wrong `getAmountOut` calculation #151

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L44 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L239

Vulnerability details

Impact

The definition of tokenOutPrice is the amount of tokenIn per tokenOut

    /// tokenIn per tokenOut price
    /// eg. 1 WBTC (8 decimals) = 40,000 CTDL ==> price = 10^8 / 40,000
    uint256 public tokenOutPrice;

In getAmountOut, _tokenInAmount is multiplied with tokenOutPrice but should be divided

        tokenOutAmount_ =
            (_tokenInAmount * tokenOutPrice) /
            tokenInNormalizationValue;

Which would lead to user receiving less token than expected and thus losing value.

Proof of Concept

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L44 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L239

eg. 1 WBTC (8 decimals) = 40,000 CTDL ==> price = 10^8 / 40,000 = 25000 getAmountOut(10e8) = (10e8 * 25000) / 10e8 = 25000 != 40000

Recommended Mitigation Steps

        tokenOutAmount_ = _tokenInAmount / tokenOutPrice;
GalloDaSballo commented 2 years ago

Disputed per the same reasoning shown in #159