Badger Citadel contest - hubble's results

Lines of code

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

Vulnerability details

There is no loss of funds, but there is a possibility that the current code will dictate user behavior which is not intended. Hence setting the risk rating as medium.

Impact

There is an option to setTokenOutPrice by the owner. There can be two cases. case-1. the new price is lower than the initial set price. If the price set for the sale is high, then users will wait until the end of the sale to buy in, in the expectation that there will be a price correction/lowering. This is not good for the protocol, because they will not be sure if the target sale will be achieved till the end.

case-2. the new price is more than the initial set price. If the initial price set for the sale is low, then users may buy out all the limit before the end of the sale duration. This is good for the protocol.

Proof of Concept

Contract : TokenSaleUpgradeable.sol

function setTokenOutPrice(uint256 _tokenOutPrice) external onlyOwner {
    require(_tokenOutPrice > 0, "TokenSale: the price must not be zero");

    tokenOutPrice = _tokenOutPrice;

    emit TokenOutPriceUpdated(_tokenOutPrice);
}

Recommended Mitigation Steps.

1. Ideally do not allow the change of tokenOutPrice during a sale duration. There will be a disparity between the early buyers and late buyers if the tokeOutPrice is changed in between sale. So, if sale target is not met, launch a new sale with lower price.

2. If the function of setTokenOutPrice is absolutely needed, allow only to set the new price to be greater than current one. This will influence the buying pattern. Users will try to buy early on before any price increase is possibly done. require(_tokenOutPrice > tokenOutPrice, "TokenSale: the price must be higher than current value");

Summary of Findings

Issue#1 : claim function should not have whenNotPaused modifier Issue#2 : finalize function should not have onlyOwner modifier

Details Issue#1

Title : claim function should not have whenNotPaused modifier

There is a possibility that the users claims can be locked (funds at risk).

Impact

If the ownerKey is compromised, then the rogue owner can pause the contract which will result in blocking the users from claiming their sale tokens.

Proof of Concept

Contract : TokenSaleUpgradeable.sol

 line 192 :     function claim() external whenNotPaused returns (uint256 tokenOutAmount_) {

Recommended Mitigation Steps

Remove the whenNotPaused modifier in the claim() function line 192 : function claim() external returns (uint256 tokenOutAmount_) {

Details Issue#2

Title : finalize function should not have onlyOwner modifier

There is a possibility that the users claims can be locked (funds at risk).

Impact

If the ownerKey is compromised, then the rogue owner can never call the finalize() function, which will result in blocking the users from claiming their sale tokens.

If the conditions of finalize() are met, then there is no risk if this function is called by anyone.

Proof of Concept

Contract : TokenSaleUpgradeable.sol

line 254 : function finalize() external onlyOwner {

Recommended Mitigation Steps

Remove the onlyOwner modifier in the finalize() function line 254 : function finalize() external {