Badger Citadel contest - OriDabush's results

Badger Citadel - Low and Noncritical Bugs:

Missing sanity check in the initialize function:

the contract can be initialized with tokenInLimit = 0, which DoS the system. Users won’t be able to buy any tokens and the system won’t work well in that case.

Forgotten TODO comment:

Before the declaration of both the TokenSaleUpgradeable and BadgerGuestlistApi contrcats there is a TODO comment which is good to mention that you forgot to remove (or to do).

Ability of setting sale’s parameters after the sale started might be unwanted:

The saleStart and saleDuration can be set after the sale has begun, which can lead to the sale to be over / not started, which might be an unwanted behavior.

Tokens will be locked in the contract when paused:

When the sale is paused, the tokens will be locked in the contract. This happens because the claim function requires that the sale isn’t paused, and the sweep function won’t transfer to the owner any tokens belonging to the users. This can be a very big problem if the contract is in a dangerous situation, for example under attack and can’t be unpaused.

Insufficient tokenOut for claimers:

The contract doesn’t promise that there will be enough tokenOut for the users to claim, so maybe it is correct to check it in the initialize function or something similar. If that is an expected behavior (for example you want to transfer tokens to the contract only after the sale) that it’s fine, but in my opinion doing it before makes the project more reliable and make the users trust it more.

Gas Optimizations in TokenSaleUpgradeable.sol

Constant expression calculation:

The expression 10**tokenOut.decimals() is calculated every time that the getAmountOut() function is called. The value of the expression doesn’t change and it can be saved to save the gas wasted on this calculation. This can be also seen in the buy function, where the expression saleStart + saleDuration is calculated every time we call it. Those 2 variables are set in the initialize function and can be set through the set functions, so the value expression mentioned above can be saved and be updated every time a new value is assigned to one of these variables.

Inline the call to getAmountOut:

The call to getAmountOut in the buy function can be replaced with the code of the function itself in order to save gas (it doesn’t mean that the getAmountOut function must be deleted). This is done to save the gas wasted on the function call when calling the getAmountOut function.

Can use unchecked in getTokenInLimitLeft function:

The getTokenInLimitLeft function do the calculation of limitLeft_ = tokenInLimit - totalTokenIn only when totalTokenIn < tokenInLimit, which promises us that there won’t be underflow, so unchecked can be used here to save the gas spending on underflow checks.