Badger Citadel contest - sirhashalot's results

Lines of code

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

Vulnerability details

Impact

In order for users to claim their promised tokenOut tokens, the contract owner must call the finalize() function. If the owner never calls the finalize() function, no user can call the claim() function to get their tokens. The owner can call the sweep() function to take all the tokens that users have sent to the contract and run away without calling finalize(). Even if the contract owner is not planning to rug users, if the owner is suddenly unresponsive, the same end result occurs where users cannot get their tokens.

I do not see any assumptions about whether the contract owner is trusted, but the contest description says "Specific care should be put in Rug Vectors". If the contract owner is not trusted, this could be a high risk issue. If the owner can be trusted to do the right thing and provide enough tokenOut tokens, then this might be a non-critical issue.

Proof of Concept

The finalize() function has the onlyOwner modifier. This provides the owner with the power to prevent users from claiming their tokenOut tokens, because the claim() function requires finalized to be true.

Recommended Mitigation Steps

Removing the onlyOwner modifier from the finalize() function so any user can start the claiming process when the sale is over. If the onlyOwner modifier remains on this function, the owner can hold the tokenOut tokens hostage.

Lines of code

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

Vulnerability details

Impact

The getAmountOut() function could result in a "tokenOutAmount_" return value of zero because solidity only supports integers and not floating point numbers. This is an unexpected edge case because the contract has require(_tokenInAmount > 0), which look like it should result in a non-zero token out amount. The result of this edge case is that the user would lose their input tokens without gaining any tokens or DAO commitments in return. The end result is a user losing tokens.

Proof of Concept

Line 221 is where the math calculating "tokenOutAmount_" could round down to zero

tokenOutAmount_ = (_tokenInAmount * 10**tokenOut.decimals()) / tokenOutPrice;

For example, if the _tokenInAmount value is 1, tokenOut.decimals() is 8, and tokenOutPrice is 10**10, then the result is:

tokenOutAmount_ = (1 * 10**8) / 10**10 = 10**8 / 10**10 = 10**-2 = 0

Any time that 10tokenOut.decimals() < tokenOutPrice would result in a tokenOutAmount_ value of zero and the user would receive no tokens despite losing their input tokens. If there is a large order of magnitude difference between the decimals of tokenOutPrice and tokenOut.decimals(), say 1010, a user could lose up to 10**10 of their tokens without getting any value from this contract.

Recommended Mitigation Steps

One simple solution for this edge case is to add a require statement to the getAmountOut() function to confirm that user will receive non-zero output tokens.

require(tokenOutAmount_ > 0);