Badger Citadel contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-02-badger-citadel/blob/08d43b5662f6d6c3707b720eadc773b913f79f2c/contracts/TokenSaleUpgradeable.sol#L254

Vulnerability details

The current implementation requires trusted key holders (Owner) to send transactions (finalize()) to finalize the sale before the buyers can claim() the tokenOut from the contract.

https://github.com/code-423n4/2022-02-badger-citadel/blob/08d43b5662f6d6c3707b720eadc773b913f79f2c/contracts/TokenSaleUpgradeable.sol#L254-L265

function finalize() external onlyOwner {
        require(!finalized, "TokenSale: already finalized");
        require(saleEnded(), "TokenSale: not finished");
        require(
            tokenOut.balanceOf(address(this)) >= totalTokenOutBought,
            "TokenSale: not enough balance"
        );

        finalized = true;

        emit Finalized();
    }

This introduces a severe centralization risk, which can cause funds to be frozen in the contract if the key holders lose access to their keys.

PoC

Given:

• 1 WBTC = 40,000 CTDL
• 100 users paid 1 WBTC each, totalTokenOutBought = 4M CTDL
• 4M CTDL tranferred to the contract

If the Owner lose access to their keys ("hit by a bus"). Then the 4M CTDL will be frozen in the contract as no one can finalize(), furthermore, all the buyers wont be able to claim() the CTDL tokens they bought.

Recommendation

Consider allowing finalize() to be called by anyone as along as the sale is ended and there is enough tokenOut balance in the contract.

Lines of code

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

Vulnerability details

In TokenSaleUpgradeable.sol#buy(), tokenIn will be transferred from the buyer directly to the saleRecipient without requiring/locking/releasing the correspoining amount of tokenOut.

This allows the saleRecipient to rug the users simply by not transferring tokenOut and finalizing the sale.

PoC

Given:

• tokenIn: WBTC
• _tokenOutPrice: 1e8
• tokenOut: CTDL

1. Alice buy() with 100e8;
2. Alice buy() with 200e8;

A malicious saleRecipient can just not transfer any CTDL to the contract and finalize() and keep 300e8 WBTC received.

As a result, Alice and Bob can not get the expected amount of tokensOut, and there is no way to retrieve the WBTC paid, in essence, lose all the funds.

Recommendation

Instead of transferring the tokenIn directing to the saleRecipient in buy(), consider transferring the tokenIn into the contract (address(this)), and require a sufficient amount of tokenOut to be transferred into the contract first before the amount of tokenIn can be released to the saleRecipient.

Overall

We find the 2022-02-badger-citadel codebase well-documented, well-structured, with a fair amount of tests, and pretty gas efficient.

There are 2 Non-critical issues and 1 Low severity issue found.

[N1] Inconsistent style of error messages

Some of the error messages are prefixed with TokenSale: while some are not.

Consider updating the error messages to keep the style of error messages consistent.

[N2] Tokens with fee on transfer are not supported

There are ERC20 tokens that charge fee for every transfer() or transferFrom().

In the current implementation, TokenSaleUpgradeable.sol#buy() assumes that the received amount is the same as the transfer amount, and uses it to calculate tokenOutAmount_.

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

Consider calling balanceOf() before and after the transfer to get the actual transferred amount if a token with transfer tax as tokenIn should be supported.

[L3] Allowing setTokenOutPrice after the token sale starts can result in unexpected results for buyers

In the current implementation, the owner can call setTokenOutPrice() and change the tokenOutPrice anytime, including when the token sale already started. In the case of network congestion or in chance, if the owner setTokenOutPrice() to a higher price, it can result in unexpected tokenOutAmount for the buyers who submitted their buy() txs before but only get packed into the block after the setTokenOutPrice() tx.

We consider this an undesirable situation for users and there is pretty much no other way to prevent it, therefore it should be prevented at the smart contract level.

Consider making setTokenOutPrice() only callable before the sale starts.

[S1] Use of upgradeable proxy is not gas efficient

Every call to an upgradable contract must be redirected by the proxy contract with an external call. Therefore, there is a gas overhead for every call to the contract.

Plus, using an upgradeable proxy make it hard to utilize some other gas optimization such as immutable variables in the constructor function.

We recommend removing the proxy and making the contract non-upgradeable.

Considering that this can save a significant amount of gas, we mark this as a Suggested level gas optimization point.

See also: [WP-H1]

[S2] Using immutable variables can save gas

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

Considering that tokenOut, tokenIn will never change, changing them to immutable variables instead of storages variable can save gas.

This optimazation depends on S1.

[S3] Cache external call results can save gas

https://github.com/code-423n4/2022-02-badger-citadel/blob/08d43b5662f6d6c3707b720eadc773b913f79f2c/contracts/TokenSaleUpgradeable.sol#L216-L224

function getAmountOut(uint256 _tokenInAmount)
    public
    view
    returns (uint256 tokenOutAmount_)
{
    tokenOutAmount_ =
        (_tokenInAmount * 10**tokenOut.decimals()) /
        tokenOutPrice;
}

Every call to an external contract costs a decent amount of gas. For optimization of gas usage, external call results should be cached if they are being used for more than one time.

tokenOut.decimals() can be cached as a storage/immutable variable, since tokenOut can not be changed, and the decimals of tokenOut are unlikely to be changed.

[S4] Using saleEnd to replace saleStart + saleDuration can save gas

https://github.com/code-423n4/2022-02-badger-citadel/blob/08d43b5662f6d6c3707b720eadc773b913f79f2c/contracts/TokenSaleUpgradeable.sol#L150-L153

https://github.com/code-423n4/2022-02-badger-citadel/blob/08d43b5662f6d6c3707b720eadc773b913f79f2c/contracts/TokenSaleUpgradeable.sol#L242

block.timestamp < saleStart + saleDuration -> block.timestamp < saleEnd

The storage variable saleDuration is commonly used with another saleStart together to compare with block.timestamp, this includes 2 SLOAD and 1 ADD.

We recommend replacing saleDuration with saleEnd, so that it can be reduced to 1 SLOAD only.

[N5] != 0 can be more gas efficient than > 0 in require statements

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

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

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

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

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

require(amount > 0, "..."); -> require(amount != 0, "...");

Considering that the difference in gas is insignificant and it cloud makes it a bit less readable, we mark this as Non-preferred.