QA Report

[code423n4]

QA Report

Table of Contents:

• QA Report
  • File: TokenSaleUpgradeable.sol (contract TokenSaleUpgradeable)
    • Contract declaration
    • Storage variables
    • function initialize()
    • function claim()
    • functions setTokenOutPrice()
    • function setSaleRecipient()
    • function setGuestlist()

File: TokenSaleUpgradeable.sol (contract TokenSaleUpgradeable)

Contract declaration

While only a comment, there an open TODO which reminds the sponsor to write better revert strings at line 13:

File: TokenSaleUpgradeable.sol
13:  * TODO: Better revert strings

As said in the gas report: revert strings are more expensive than custom errors, therefore you should use them instead (they exist since Solidity 0.8.4). If you decide to keep revert strings, make sure their length don't exceed 32 characters.

Also, don't forget to delete the comment before deploying, of course.

Storage variables

Related data should be grouped in a struct: For maps that use the same key value: having separate fields is error prone (like in case of deletion or future new fields).

In this contract, those 3 maps use the same key:

File: TokenSaleUpgradeable.sol
36:     mapping(address => uint256) public boughtAmounts;
...
40:     mapping(address => bool) public hasClaimed;
...
57:     mapping(address => uint8) public daoVotedFor;

Proof:

contracts\TokenSaleUpgradeable.sol:164:        uint256 boughtAmountTillNow = boughtAmounts[msg.sender];
contracts\TokenSaleUpgradeable.sol:168:                _daoId == daoVotedFor[msg.sender],
contracts\TokenSaleUpgradeable.sol:172:            daoVotedFor[msg.sender] = _daoId;
contracts\TokenSaleUpgradeable.sol:177:        boughtAmounts[msg.sender] = boughtAmountTillNow + tokenOutAmount_;
contracts\TokenSaleUpgradeable.sol:193:        require(!hasClaimed[msg.sender], "already claimed");
contracts\TokenSaleUpgradeable.sol:195:        tokenOutAmount_ = boughtAmounts[msg.sender];
contracts\TokenSaleUpgradeable.sol:199:        hasClaimed[msg.sender] = true;

I'd suggest these 3 related data get grouped in a struct, let's name it AccountInfo:

struct AccountInfo {
  uint256 boughtAmounts;
  bool hasClaimed;
  uint8 daoVotedFor;
}

And it would be used as a state variable in this manner:

mapping(address => AccountInfo) accountInfo;

Even if you could then delete all related fields with a simple delete accountInfo[address], I understand that boughtAmounts needs to be persisted. Still, for maintainability and consistency, grouping these related data together is a good practice.
Furthermore, the tight-packing of structs (2 storage slots as uint256 is 32 bytes, bool is 1 byte and uint8 is 1 byte) would save gas.

function initialize()

1. Front-Runnable Initializers

The onlyOwner modifier is missing (the default owner of a contract is the one who creates it). This missing access controls allows any user to initialize the contract. By front-running, the incorrect parameters may be supplied, leaving the contract needing to be redeployed (there are no setters for _tokenOut and _tokenIn, and initialize can only be called once because of the initializer modifier).

I suggest adding the missing onlyOwner modifier

2. Missing address(0) check on _tokenOut
3. Missing address(0) check on _tokenIn

As these addresses don't have setters, an error would result in the contract needing to be redeployed.

4. Missing 0 check on _tokenInLimit

While this can be corrected by calling setTokenInLimit: _tokenInLimit should be 0 checked as this would instantly end sales (Line 243). The 0 check should also be added in setTokenInLimit as this would make the owner able to instant-terminate the sales, which is a permission that requires a lot of trust from users. Furthermore, adding a timelock behind setTokenInLimit is a Medium level issue that you should consider.

5. Confusing revert string: "TokenSale: start date may not be in the past"

The first thing someone can understand is that the error is due to the fact that the start date isn't in the past. But the revert string's "may" isn't the probability one, but the permission one, which actually means the opposite. It would be clearer to write: "TokenSale: start date cannot be in the past" or "TokenSale: start date shouldn't be in the past". This correction can also be applied in the setSaleStart() function (line 274).

6. _saleDuration should have a lower boundary

While _saleDuration is indeed 0 checked, this require statement can easily fail almost instantly:

File: TokenSaleUpgradeable.sol
150:         require(
151:             block.timestamp < saleStart + saleDuration,
152:             "TokenSale: already ended"
153:         );

Consider adding a lower boundary to _saleDuration in initialize() and setSaleDuration().

function claim()

1. Missing comment: "@return tokenOutAmount_ Amount of tokenOut bought"

https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/TokenSaleUpgradeable.sol#L188-L190

functions setTokenOutPrice()

1. tokenOutPrice should have an upper-boundary

While it may seem unlikely to set it that way, tokenOutPrice can take the max value of uint256 (line 306):

File: TokenSaleUpgradeable.sol
303:     function setTokenOutPrice(uint256 _tokenOutPrice) external onlyOwner { //@audit timelock needed
304:         require(_tokenOutPrice > 0, "TokenSale: the price must not be zero");
305:          //@audit where is require(!finalized)?
306:         tokenOutPrice = _tokenOutPrice; //@audit should have upper boundary
307: 
308:         emit TokenOutPriceUpdated(_tokenOutPrice);
309:     }

This would make the "amount received when exchanging tokenIn" drop to a negligible amount:

File: TokenSaleUpgradeable.sol
216:     function getAmountOut(uint256 _tokenInAmount)
217:         public
218:         view
219:         returns (uint256 tokenOutAmount_) 
220:     {
221:         tokenOutAmount_ =
222:             (_tokenInAmount * 10**tokenOut.decimals()) /
223:             tokenOutPrice; //@audit tokenOutPrice can take the max value from uint256
224:     }

I believe an upper-boundary is needed, this would also be good for users' trust.

2. Missing require(!finalized, "TokenSale: already finalized");

function setSaleRecipient()

1. Missing require(!finalized, "TokenSale: already finalized");

function setGuestlist()

1. Missing require(!finalized, "TokenSale: already finalized");

[0xleastwood]

The rest of these issues I agree with. However, I don't think the initialize function is front-runnable. This contract is intended to be upgradeable through a proxy setup. The initialize function essentially acts as the constructor.