Badger Citadel contest - gzeon's results

Lines of code

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

Vulnerability details

Impact

The setTokenOutPrice function allow owner to update the tokenOutPrice. The owner of the contract can potential "rug" users by front-runing buy transactions to set a very high price. Since the README file specified "Rug Vectors" in the scope, while this attack require privileged access it should still be taken care of.

Proof of Concept

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

    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. Remove setTokenOutPrice function ; or
2. Add a maxBuyPrice parameter to buy function

Initialization function can be front-run

Anyone can run the initialize function https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L96

tokenInLimit can be set to lower than totalTokenIn

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

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

        tokenInLimit = _tokenInLimit;

        emit TokenInLimitUpdated(_tokenInLimit);
    }

initialize lack of input validation

• _tokenOut and _tokenIn are not made sure !=address(0)
• _tokenInLimit is not made sure !=0

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

No event emited by initialize

might consider emit these event in initialize to allow tracking of these parameters

    event SaleStartUpdated(uint64 saleStart);
    event SaleDurationUpdated(uint64 saleDuration);
    event TokenOutPriceUpdated(uint256 tokenOutPrice);
    event SaleRecipientUpdated(address indexed recipient);
    event GuestlistUpdated(address indexed guestlist);
    event TokenInLimitUpdated(uint256 tokenInLimit);

Missing dao whitelist

_daoId can be anything within the range of uint8, might consider adding a mapping of votable dao

> 0 is less efficient than != 0 for uint in require condition

Ref: https://twitter.com/GalloDaSballo/status/1485430908165443590 https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L111

        require(
            _saleDuration > 0,
            "TokenSale: the sale duration must not be zero"
        );

grep -Rn "> 0" ./contracts/ | grep require
./contracts/TokenSaleUpgradeable.sol:114:        require(_tokenOutPrice > 0, "TokenSale: the price must not be zero");
./contracts/TokenSaleUpgradeable.sol:154:        require(_tokenInAmount > 0, "_tokenInAmount should be > 0");
./contracts/TokenSaleUpgradeable.sol:197:        require(tokenOutAmount_ > 0, "nothing to claim");
./contracts/TokenSaleUpgradeable.sol:304:        require(_tokenOutPrice > 0, "TokenSale: the price must not be zero");
./contracts/TokenSaleUpgradeable.sol:364:        require(amount > 0, "nothing to sweep");

Use custom errors

Solidity ^0.8.4 allow the use of custom errors to optimize gas usage. https://blog.soliditylang.org/2021/04/21/custom-errors/

Use uint256 instead of bool

Use uint(1) instead of bool(true) to save gas by avoiding masking ops https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L40

    mapping(address => bool) public hasClaimed;

Use uint256 instead of uint8

Use uint256 instead of uint8 to save gas by avoiding masking ops https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L57

    mapping(address => uint8) public daoVotedFor;

float multiplication optimization

// out = x * y unchecked{/} z
function fmul(uint256 x, uint256 y, uint256 z) internal pure returns(uint256 out){
assembly{
    if iszero(eq(div(mul(x,y),x),y)) {revert(0,0)}
    out := div(mul(x,y),z)
}
}

can be applied to floating multiplication such as in https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L221

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

Unchecked safe math

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

        boughtAmounts[msg.sender] = boughtAmountTillNow + tokenOutAmount_;
        daoCommitments[_daoId] += tokenOutAmount_;

        totalTokenIn += _tokenInAmount;
        totalTokenOutBought += tokenOutAmount_;

can be changed into

        totalTokenOutBought += tokenOutAmount_;

        unchecked{
            boughtAmounts[msg.sender] = boughtAmountTillNow + tokenOutAmount_;
            daoCommitments[_daoId] += tokenOutAmount_;
            totalTokenIn += _tokenInAmount;
        }

since

1. checked totalTokenOutBought will not overflow in L156
2. if totalTokenOutBought does not overflow, boughtAmounts[msg.sender] and daoCommitments[_daoId] will not overflow