Badger Citadel contest - IllIllI's results

Lines of code

https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L216-L224

Vulnerability details

Impact

The contract comments nor the submission text detail what happens when the user submits more tokenIn tokens than are required to recieve N tokenOut tokens, but not enough for N+1 tokens. The contract as written takes any excess contributions for itself.

Impact

Due to loss of precision, the number of tokenOut tokens receieved is rounded down and the user loses any excess contributions, and also has to deal with any tax implications, due to this undocumented and surprising behavior. If amount of funds lost is directly proportional to the ratio of the tokenOutPrice to tokenOut.decimals().

Proof of Concept

If tokenOutPrice is greater than 10**tokenOut.decimals(), loss of precision due to tokenOutAmount_ not being a floating type, will mean that the caller will lose out on fractions of tokenOut tokens. The buy() function does not return any excess tokenIn tokens, and does not keep track of fractions of tokens partially paid for, and instead keeps them for itself.

buy() calls getAmountOut():

        tokenOutAmount_ = getAmountOut(_tokenInAmount);

        boughtAmounts[msg.sender] = boughtAmountTillNow + tokenOutAmount_;
        daoCommitments[_daoId] += tokenOutAmount_;

        totalTokenIn += _tokenInAmount;
        totalTokenOutBought += tokenOutAmount_;

https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L175-L181

And getAmount() doesn't keep track of partial tokenOutAmount_s

    function getAmountOut(uint256 _tokenInAmount)
        public
        view
        returns (uint256 tokenOutAmount_)
    {
        tokenOutAmount_ =
            (_tokenInAmount * 10**tokenOut.decimals()) /
            tokenOutPrice;
    }

https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L216-L224

Based on this conversion comment

/// eg. 1 WBTC (8 decimals) = 40,000 CTDL ==> price = 10^8 / 40,000

https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L32

If tokenOut were something like a BTC with only 2 decimals and tokenIn were USD, the comment would read:

/// eg. 40,000 USD (2 decimals) = 1 BTC ==> price = (40,000 * 10^2) / 1

which is 4,000,000. With this tokenOutPrice, if someone submitted $79k USDC, they'd get int((79000 * 10**2)/(4000000) which is only 1 BTC, when they spent almost 2 BTC's worth of USDC.

Tools Used

Code inspection

Recommended Mitigation Steps

The code should follow the "principle of least surprise" and require() that tokenOutPrice be less than or equal to 10**tokenOut.decimals(), or keep track of partial amounts, or refund excess contributions.

Handle: IllIllI

Risk rating: Gas Optimizations

Gas report: See markdown file here