Badger Citadel contest - Dravee's results

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");

Gas Report

Table of Contents:

• Gas Report
  • File: TokenSaleUpgradeable.sol
    • Foreword
    • function buy()
    • function getTokenInLimitLeft()
  • General recommendations
    • > 0 is less efficient than != 0 for unsigned integers (with proof)
    • Reduce the size of error messages (Long revert Strings)
    • Use Custom Errors instead of Revert Strings to save Gas

File: TokenSaleUpgradeable.sol

Foreword

• Storage-reading optimizations

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). In the paragraphs below, please see the @audit-issue tags in the pieces of code's comments for more information about SLOADs that could be saved by caching the mentioned storage variables in memory variables.

• Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

function buy()

The code is as such:

File: TokenSaleUpgradeable.sol
149:         require(saleStart <= block.timestamp, "TokenSale: not started"); //@audit-issue saleStart SLOAD 1
150:         require(
151:             block.timestamp < saleStart + saleDuration,//@audit-issue saleStart SLOAD 2
152:             "TokenSale: already ended"
153:         );
154:         require(_tokenInAmount > 0, "_tokenInAmount should be > 0"); 
155:         require(
156:             totalTokenIn + _tokenInAmount <= tokenInLimit,//@audit-issue totalTokenIn SLOAD 1
157:             "total amount exceeded"
158:         );
159: 
160:         if (address(guestlist) != address(0)) { //@audit-issue guestlist SLOAD 1
161:             require(guestlist.authorized(msg.sender, _proof), "not authorized"); //@audit-issue guestlist SLOAD 2
162:         }
163: 
164:         uint256 boughtAmountTillNow = boughtAmounts[msg.sender];
165: 
166:         if (boughtAmountTillNow > 0) {
167:             require(
168:                 _daoId == daoVotedFor[msg.sender],
169:                 "can't vote for multiple daos"
170:             );
171:         } else {
172:             daoVotedFor[msg.sender] = _daoId;
173:         }
174: 
175:         tokenOutAmount_ = getAmountOut(_tokenInAmount);
176: 
177:         boughtAmounts[msg.sender] = boughtAmountTillNow + tokenOutAmount_;
178:         daoCommitments[_daoId] += tokenOutAmount_;
179: 
180:         totalTokenIn += _tokenInAmount;//@audit-issue totalTokenIn SLOAD 2

By caching in memory:

• guestlist
• totalTokenIn
• saleStart

It's possible to save 3 SLOADs (a little less than 300 gas by taking into account the MSTOREs+MLOADs)

function getTokenInLimitLeft()

The code is as such:

File: TokenSaleUpgradeable.sol
230:     function getTokenInLimitLeft() external view returns (uint256 limitLeft_) {
231:         if (totalTokenIn < tokenInLimit) { //@audit-issue totalTokenIn SLOAD 1 //@audit-issue tokenInLimit SLOAD 1
232:             limitLeft_ = tokenInLimit - totalTokenIn; //@audit-issue totalTokenIn SLOAD 2 //@audit-issue tokenInLimit SLOAD 2 //@audit-issue should be unchecked
233:         }
234:     }

1. By caching in memory:

• totalTokenIn
• tokenInLimit

It's possible to save 2 SLOADs (a little less than 200 gas)

2. As totalTokenIn < tokenInLimit, it's impossible for tokenInLimit - totalTokenIn to underflow. Therefore:

Line 232 should be wrapped inside an unchecked block.

General recommendations

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

> 0 in require statements are used in the following location(s):

contracts\TokenSaleUpgradeable.sol:111:            _saleDuration > 0,
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:289:            _saleDuration > 0,
contracts\TokenSaleUpgradeable.sol:304:        require(_tokenOutPrice > 0, "TokenSale: the price must not be zero");
contracts\TokenSaleUpgradeable.sol:364:        require(amount > 0, "nothing to sweep");

I suggest you change > 0 with != 0 in require statements. Also, enable the Optimizer.

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes are here:

TokenSaleUpgradeable.sol:108:            "TokenSale: start date may not be in the past"
TokenSaleUpgradeable.sol:112:            "TokenSale: the sale duration must not be zero"
TokenSaleUpgradeable.sol:114:        require(_tokenOutPrice > 0, "TokenSale: the price must not be zero");
TokenSaleUpgradeable.sol:117:            "TokenSale: sale recipient should not be zero"
TokenSaleUpgradeable.sol:274:            "TokenSale: start date may not be in the past"
TokenSaleUpgradeable.sol:290:            "TokenSale: the sale duration must not be zero"
TokenSaleUpgradeable.sol:304:        require(_tokenOutPrice > 0, "TokenSale: the price must not be zero");
TokenSaleUpgradeable.sol:318:            "TokenSale: sale recipient should not be zero"

I suggest you shorten the revert strings to fit in 32 bytes, or that you use custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

TokenSaleUpgradeable.sol:108:            "TokenSale: start date may not be in the past"
TokenSaleUpgradeable.sol:112:            "TokenSale: the sale duration must not be zero"
TokenSaleUpgradeable.sol:117:            "TokenSale: sale recipient should not be zero"
TokenSaleUpgradeable.sol:152:            "TokenSale: already ended"
TokenSaleUpgradeable.sol:157:            "total amount exceeded"
TokenSaleUpgradeable.sol:169:                "can't vote for multiple daos"
TokenSaleUpgradeable.sol:259:            "TokenSale: not enough balance"
TokenSaleUpgradeable.sol:274:            "TokenSale: start date may not be in the past"
TokenSaleUpgradeable.sol:290:            "TokenSale: the sale duration must not be zero"
TokenSaleUpgradeable.sol:318:            "TokenSale: sale recipient should not be zero"

I suggest you replace revert strings with custom errors.