Badger Citadel contest - hyh's results

Lines of code

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

Vulnerability details

Impact

Sale participants have to bear the full risk of owner misbehavior or just a technical malfunction until the very end of the sale.

For example, if owner's private key is somehow lost, all participants deposits to be gone if tokenOut isn't yet transferred to the contract.

tokenOut transfer is required for the sale finalization only, which isn't obliged to happen, while tokenIns are transferred to the receiver right away.

I.e. current setup requires sale participants' full faith in the contract owner and makes them face full spectrum of the technical risks, not dealing with them anyhow, while it is straightforward to limit them (and, some participants might ask why is it structured this way).

Proof of Concept

The only requirement for CTDL tokens to be present in the contract is in the finalization function, which simply can be never called in the bad scenario:

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

Recommended Mitigation Steps

Consider adding the check in the initialize function for tokenInLimit to be positive and require there that (tokenInLimit * 10**tokenOut.decimals()) / tokenOutPrice of tokenOut should be transferred to the contract right away.

Now:

function initialize(
...
	tokenOutPrice = _tokenOutPrice;
	tokenInLimit = _tokenInLimit;

To be:

function initialize(
...
	require(_tokenInLimit > 0, ...)
	require(tokenOut.safeTransferFrom(msg.sender, address(this), (_tokenInLimit * 10**tokenOut.decimals()) / _tokenOutPrice));
	
	tokenOutPrice = _tokenOutPrice;
	tokenInLimit = _tokenInLimit;

1. Initialize can be front run (low)

Impact

As initialize can be run by anyone an attacker can monitor the contract construction and then initialize the contract before the deplorer by front running the second, initialization, transaction. As an attacker will full control of the system after that, the contract have to be redeployed if this is noticed early enough. Otherwise the attacker will have access to all the funds sent to the contract

Proof of Concept

Initialize function doesn't have control modifiers, can be run by anyone:

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

Recommended Mitigation Steps

The easiest way to handle the issue is to ensure that initialize is run atomically within one transaction with TokenSaleUpgradeable constructor

2. Leftover debug comments (non-critical)

Proof of Concept

boughtAmounts aren't updated after a claim, there is no need to comment on the possibility (i.e. should go to private dev notes, not to public prod code):

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

TODOs also shouldn't go to production to keep the code clean:

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

Proof of Concept

Consider removing non-prod comments

3. tokenOutPrice precision can be enhanced (low)

Impact

tokenOutPrice constructed as {asset decimals} / {asset USD price} means the effective asset price is rounded, which opens the way to sale manipulation to an extent, i.e. participants will use the contract that rounds the price the most in their favor, leading to decreasing sale output.

For example, if current market of WBTC is $100k, tokenOutPrice will be the same for [99951, 100050] range: int(10^8/99950) = int(10^8/100050) = 1000

Proof of Concept

tokenOutPrice to be constructed as {asset decimals} / {asset USD price} (WBTC: 10^8 / 40000):

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

Recommended Mitigation Steps

Add a decimals enhancing multiplier, say 10^18, so the price to be {asset decimals} * {priceMultiplier} / {asset USD price}

Update getAmountOut accordingly:

Now:

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

To be:

function getAmountOut(uint256 _tokenInAmount)
...
  tokenOutAmount_ = (_tokenInAmount * priceMultiplier * 10**tokenOut.decimals()) / tokenOutPrice;

1. authorized function can be simplified

Impact

Code is overcomplicated for the current functionality; its gas cost is increased.

Proof of Concept

https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/utils/VipGuestListUpgradeable.sol#L89-111

Recommended Mitigation Steps

Now:

bool invited = guests[_guest];

if (!invited && guestRoot == bytes32(0)) {
		invited = true;
}

if (!invited && guestRoot != bytes32(0)) {
		invited = _verifyInvitationProof(_guest, _merkleProof);
}

return invited;

Consider a simpler approach:

if (guests[_guest]) return true;

if (guestRoot == bytes32(0)) return true;

return _verifyInvitationProof(_guest, _merkleProof);

2. buy function dao logic can be simplified

Impact

Gas is overspent on check and revert

Proof of Concept

It is enough to sate in the sale documentation that dao vote to be recorder first buy only and later values to be ignored. It's effectively the same output with less costs.

Now checks are done all the time:

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

Recommended Mitigation Steps

Now:

if (boughtAmountTillNow > 0) {
   require(
      _daoId == daoVotedFor[msg.sender], "can't vote for multiple daos");
} else {
      daoVotedFor[msg.sender] = _daoId;
}

To be:

// Only first DAO vote is valid
if (boughtAmountTillNow == 0) daoVotedFor[msg.sender] = _daoId;

3. Use custom errors to save gas

Impact

Gas is overspent on requires

Proof of Concept

solidity 0.8.11 allows for custom errors:

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

https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/utils/VipGuestListUpgradeable.sol#L2

Recommended Mitigation Steps

Consider introducing and using custom errors instead of the revert strings to save gas:

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

4. Token decimals should be saved

Impact

Gas is overspent on tokenOut.decimals() call

Proof of Concept

tokenOut.decimals() are called on each buy function call, which is frequent enough to save it to storage:

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

Recommended Mitigation Steps

Save tokenOut.decimals() to storage on initialization and use it thereafter