Badger Citadel contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/TokenSaleUpgradeable.sol#L271-L281 https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/TokenSaleUpgradeable.sol#L287-L297 https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/TokenSaleUpgradeable.sol#L340-L346

Vulnerability details

Impact

Sale would never end and Owner can transfer all user tokens using the sweep function

Proof of Concept

1. Observe the functions setSaleDuration, setSaleStart, setTokenInLimit

2. Any of these functions can be called in between of sale

3. So if Sale was meant to be started on timestamp 100 and would end at timestamp 500. Owner can simply call setSaleDuration and set large duration say 100000

4. Since sale duration increased too much (100100), finalize could never be called which also means user cannot claim any amount

5. On the other hand owner is free to use sweep function and could transfer all the tokenIn tokens which user paid to the contract while buying

6. Same scenario could also happen using setSaleStart, setTokenInLimit function. Setting large values for these will make sure that sale never ends

Recommended Mitigation Steps

Add a check to only allow changes if sale has not started yet in functions setSaleDuration, setSaleStart, setTokenInLimit

require(saleStart > block.timestamp, "Sale has already started");

Total 5 Low issue identified

Input validation missing in setTokenInLimit

1. Observe the setTokenInLimit function

function setTokenInLimit(uint256 _tokenInLimit) external onlyOwner {
        require(!finalized, "TokenSale: already finalized");

        tokenInLimit = _tokenInLimit;

        emit TokenInLimitUpdated(_tokenInLimit);
    }

2. Observe that there is no check to see that new _tokenInLimit > totalTokenIn.

3. This is very much required as current totalTokenIn could already be equal to tokenInLimit. Decreasing tokenInLimit puts contract in an invalid state where totalTokenIn > tokenInLimit which should not be possible ideally.

Remediation: Change function like below:

function setTokenInLimit(uint256 _tokenInLimit) external onlyOwner {
        ...
        require(totalTokenIn <= _tokenInLimit , "Incorrect token limit");
        ...
    }

Contract comment not followed in function

1. Observe the setTokenOutPrice function

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

        tokenOutPrice = _tokenOutPrice;

        emit TokenOutPriceUpdated(_tokenOutPrice);
    }

2. This contradicts the comment "@notice Sells a token at a predetermined price to whitelisted buyers." Since prices will change (getAmountOut changes) if Owner changes setTokenOutPrice in middle of sale, hence buyers will get lesser tokens if Owner decides to increase tokenoutprice in middle of sale

Remediation: Change the function like below:

function setTokenOutPrice(uint256 _tokenOutPrice) external onlyOwner {
        ...
        require(saleStart > block.timestamp, "Price Locked");
        ...
    }

Unfair Guest List

1. Observe the setGuestlist function

function setGuestlist(address _guestlist) external onlyOwner {
        guestlist = BadgerGuestListAPI(_guestlist);

        emit GuestlistUpdated(_guestlist);
    }

2. Observe that owner can update the Guest list at any point of sale.

3. Ideally the users who were whitelisted before sale start should be in guestlist and owner should not be able to add new whitelisted user after sale starts

Remediation: Add a check:

require(saleStart > block.timestamp, "Sale has started, Guest Locked");

Changing start date could cause Panic

1. Observe the function setSaleStart

function setSaleStart(uint64 _saleStart) external onlyOwner {
        require(
            _saleStart >= block.timestamp,
            "TokenSale: start date may not be in the past"
        );
        require(!finalized, "TokenSale: already finalized");

        saleStart = _saleStart;

        emit SaleStartUpdated(_saleStart);
    }

2. Observe that owner can change SaleStart date even if sale has already started.

3. If sale has already started then this function should not be allowed and setSaleDuration should be used instead as this could cause panic in existing sale buyers

Remediation: Add a check:

require(saleStart > block.timestamp, "Sale has already started");

No check to see that _daoId is existing

1. Observe that in buy function, any arbitrary _daoId is accepted and updated in daoVotedFor, daoCommitments
2. Ideally contract should check if the provided _daoId is valid or not