Badger Citadel contest - 0x0x0x's results

buy can charge the user extra

When a user uses buy function, it pays as much as _tokenInAmount. This approach charges users with extra rounding error. If this rounding error is really small with planned parameter, this is a non-critical problem. If it can be more than extra gas cost of calculating exact, I recommend to do so.

setSaleRecipient posses extra risk

In case the access of owner address gets in control of an hacker, this creates a possibility of theft from buyers. Without setSaleRecipient, the hacker can directly steal funds and also even if output tokens are stolen by using sweep, it will be easy to keep track of it. For users security, I recommend removing this function.

No upper limit for tokenOutPrice

tokenOutPrice can be set to maximum value possible. This can be used to make users pay for nothing. In case the owner is not a timelock contract, it is possible to change the price and scam users buying tokens. I recommend having a reasonable maximum cap for tokenOutPrice to provide better security for users. This will also only cost gas for owners, which is likely not important.

No slippage protection

buy function does not have any functionality for slippage protection. Since setTokenOutPrice can change the price, it can result in buy orders for an unexpected rate.

Furthermore, if there is enough allowance and the owners might be able to change the price to swap the funds for almost no token.

Mitigation

For users security and best practice, I recommend adding a new input variable to function buy, which can be used to specify the minimum output. Most popular contracts use this design pattern to avoid such problems.

Save to a global variable tokenOut.decimals()

Save gas in getAmountOut and also eliminate the possibility of price manipulations by changing decimals.

In buy, totalTokenIn + _tokenInAmount is calculated twice

L155-158:

require(
            totalTokenIn + _tokenInAmount <= tokenInLimit, //@audit gas
            "total amount exceeded"
        );

L189:

totalTokenIn += _tokenInAmount;

If the addition at L189 is performed before this require statement, there is no need to add _tokenInAmount anymore.

Cache saleStart and guestlist in buy

Both variables are loaded twice. Therefore caching them can save gas.

sweep can be optimized to save gas

Since amountLeftToBeClaimed is only used once, it is not needed to use this variable.

        if (_token == address(tokenOut)) {
            uint256 amountLeftToBeClaimed = totalTokenOutBought -
                totalTokenOutClaimed;
            amount -= amountLeftToBeClaimed;
        }

can be replaced with

        if (_token == address(tokenOut)) {
            amount -= totalTokenOutBought - totalTokenOutClaimed;
        }