QA report

[code423n4]

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.

[GalloDaSballo]

Ultimately the rounding issue is a problem with integer division, I don't think the finding as dramatic as it sounds

[0xleastwood]

Duplicate of #50

[0xleastwood]

After further thought, I actually think this is more of an issue of state handling. Users can mis-use this function to receive slightly lower amounts than expected due to the imprecise nature of arithmetic in Solidity. I'll mark this as low to reflect this.

[CloudEllie]

Since this issue was downgraded to a QA level, and the warden did not submit a separate QA report, we've renamed this one to "QA report" for consistency.

The original title, for the record, was "Loss of excess funds due to loss of precision."