Badger Citadel contest - defsec's results

Lines of code

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

Vulnerability details

Impact

During the documentation of the contest, It has been seen that the following comment added.

Specific care should be put in:

Economic exploits
Rug Vectors

However, onlyOwner function does not have upper bound definition on the related variables. Values that are too large will lead to reversions in several critical functions. For instance : claim() Function.

Proof of Concept

1. Navigate to the following functions.

setSaleDuration function

    function setSaleDuration(uint64 _saleDuration) external onlyOwner {
        require(
            _saleDuration > 0,
            "TokenSale: the sale duration must not be zero"
        );
        require(!finalized, "TokenSale: already finalized");

        saleDuration = _saleDuration;

        emit SaleDurationUpdated(_saleDuration);
    }

setTokenInLimit function

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

        tokenInLimit = _tokenInLimit;

        emit TokenInLimitUpdated(_tokenInLimit);
    }

2. The function does not have any upper bound on the variables. The claim function is only works when the finalized variable is set to true.

    function claim() external whenNotPaused returns (uint256 tokenOutAmount_) {
        require(finalized, "sale not finalized");
....

3. The finalize variable is set in the following function.

    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();
    }

4. With the malicious owner action, the variable will be defined as larger and user funds will be locked in the contract.

Tools Used

Code Review

Recommended Mitigation Steps

Consider to define upper bounds on the setter function variables. The contract can have emergencyWithdraw function in the related function. That can save user fund in the unexcepted behaviours. The reasonable timelock should be added into the setter function.

C4-001 : transferOwnership should be two step process

Impact - LOW

"TokenSaleUpgradeable.sol" inherit OpenZeppelin's OwnableUpgradeable contract which enables the onlyOwner role to transfer ownership to another address. It's possible that the onlyOwner role mistakenly transfers ownership to the wrong address, resulting in a loss of the onlyOwner role. The current ownership transfer process involves the current owner calling Unlock.transferOwnership(). This function checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier. Lack of two-step procedure for critical operations leaves them error-prone if the address is incorrect, the new address will take on the functionality of the new role immediately

for Ex : -Alice deploys a new version of the whitehack group address. When she invokes the whitehack group address setter to replace the address, she accidentally enters the wrong address. The new address now has access to the role immediately and is too late to revert

Proof of Concept

1. Navigate to "https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/TokenSaleUpgradeable.sol#L15"
2. The contracts have many onlyOwner function.
3. The contract is inherited from the Ownable which includes transferOwnership.

Tools Used

None

Recommended Mitigation Steps

Implement zero address check and Consider implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

C4-002 : Missing zero-address check in constructors and the setter functions

Impact - LOW

Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly.

Proof of Concept

1. Navigate to the following contract functions.

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

Variables

• _tokenOut

• _tokenIn

• _guestlist

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

Variables

• guestlist

Tools Used

Code Review

Recommended Mitigation Steps

Consider adding zero-address checks in the discussed constructors: require(newAddr != address(0));.

C4-003 : Front-runnable Initializers

Impact - LOW

All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.

Proof of Concept

1. Navigate to the following contracts.

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

2. initialize functions does not have access control. They are vulnerable to front-running.

Tools Used

Manual Code Review

Recommended Mitigation Steps

While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers would be a sufficient level of access control.

C4-004 : Incompatibility With Rebasing/Deflationary/Inflationary tokens

Impact - LOW

BadgerDAO protocol do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.

Proof of Concept

1. Navigate to the following contract.

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

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

Tools Used

Manual Code Review

Recommended Mitigation Steps

• Ensure that to check previous balance/after balance equals to amount for any rebasing/inflation/deflation
• Add support in contracts for such tokens before accepting user-supplied tokens
• Consider supporting deflationary / rebasing / etc tokens by extra checking the balances before/after or strictly inform your users not to use such tokens if they don't want to lose them.

C4-005 : Use of Block.timestamp

Impact - Non-Critical

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Proof of Concept

1. Navigate to the following contract.

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

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

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

Tools Used

Manual Code Review

Recommended Mitigation Steps

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

C4-006 : Missing Re-entrancy Guard

Impact - Non-Critical

Consider using ReentrancyGuard to protect functions that have external calls and do not follow Checks Effects Interactions pattern. An example of a function that needs to prevent re-entrancy is buy as it calls claim before updating the state and because anyone can add new bounties with no restrictions, it may contain tokens with callbacks on transfer (e.g. erc777) which may call this function again and again.

Proof of Concept

1. Navigate to the following contract function.

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

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

2. The functions are missing re-entrancy guard.

Tools Used

Manual Code Review

Recommended Mitigation Steps

Consider implementing re-entrancy guard on the functions. Use the OpenZeppelin ReentrancyGuard.sol on the main functions users will interact with such as buy & claim.(https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol)

C4-007 : Open TODOs

Impact - Non-Critical

Open TODOs can point to architecture or programming issues that still need to be resolved. Timestamp in the Vaderpool should be re-evaluated.

Proof of Concept

1. Navigate to the following contract.

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

Tools Used

Manual Code Review

Recommended Mitigation Steps

Consider resolving the TODOs before deploying.

C4-008 : No standard has been defined on the events

Impact - Non-Critical

In the TokenSaleUpgradeable contract, there is no standard has been defined. Some of contract function has "TokenSale::" event statement and some of them do not have.

Proof of Concept

1. Navigate to the following contract functions.

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

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

2. Two function have same checks for the finalize variable. However, the event structure is not same.

Tools Used

Manual Code Review

Recommended Mitigation Steps

It is recommended to define same structure on the events.

C4-001 : Adding unchecked directive can save gas

Impact - Gas Optimization

Using the unchecked keyword to avoid redundant arithmetic underflow/overflow checks to save gas when an underflow/overflow cannot happen. E.g. 'unchecked' can be applied in the following lines of code since there are require statements before to ensure the arithmetic operations would not cause an integer underflow or overflow. For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

Proof of Concept

1. Navigate to the following contract function and lines.

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

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

Tools Used

Code Review

Recommended Mitigation Steps

Consider applying unchecked arithmetic where overflow/underflow is not possible.

C4-002 : Revert String Size Optimization

Impact - Gas Optimization

Shortening revert strings to fit in 32 bytes will decrease deploy time gas and will decrease runtime gas when the revert condition has been met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of Concept

1. Navigate to the following contract function and lines.

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#L274

Tools Used

Code Review

Recommended Mitigation Steps

Shorten the revert strings to fit in 32 bytes. That will affect gas optimization.

C4-003 : Immutable variables

Impact - Gas Optimization

'immutable' greatly reduces gas costs. There are variables that do not change so they can be marked as immutable to greatly improve the gas costs.

Proof of Concept

1. Navigate to the following contract function and lines.

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

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

2. The following variables can be defined as an immutable.

    /// token to give out (CTDL)
    ERC20Upgradeable public tokenOut;
    /// token to take in WBTC / bibbtc LP / CVX / bveCVX
    ERC20Upgradeable public tokenIn;

Tools Used

Code Review

Recommended Mitigation Steps

Define these state variables as immutables.

C4-004 : > 0 can be replaced with != 0 for gas optimization

Impact - Gas Optimization

!= 0 is a cheaper operation compared to > 0, when dealing with uint.

Proof of Concept

1. Navigate to the following contract function and lines.

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#L111

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#L154

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

Tools Used

Code Review

Recommended Mitigation Steps

Use "!=0" instead of ">0" for the gas optimization.

C4-005 : Use else if for mutually exclusive conditions

Impact - Gas Optimization

Use else if for mutually exclusive conditions to save gas.

Proof of Concept

1. Navigate to the following contract function line.

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

Tools Used

Code Review

Recommended Mitigation Steps

Consider using if/else-if/else statement instead of multiple if statement.

C4-006 : Less than 256 uints are not gas efficient

Impact - Gas Optimization

Lower than uint256 size storage instance variables are actually less gas efficient. E.g. using uint16 does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint16 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.

Proof of Concept

1. Navigate to the following contract function line.

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

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

Tools Used

Code Review

Recommended Mitigation Steps

Consider to review all uint types. Change them with uint256 If the integer is not necessary to present with uint64.

C4-007 : Changing function visibility from public to external can save gas

Impact - Gas Optimization

There is a function declared as public that are never called internally within the contract. It is best practice to mark such functions as external instead, as this saves gas (especially in the case where the function takes arguments, as external functions can read arguments directly from calldata instead of having to allocate memory).

Proof of Concept

1. Navigate to the following contract function line.

renounceOwnership() should be declared external:                                                                                                           
        - OwnableUpgradeable.renounceOwnership() (lib/openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol#60-62)                    
transferOwnership(address) should be declared external:                                                                                                    
        - OwnableUpgradeable.transferOwnership(address) (lib/openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol#68-71)             
name() should be declared external:                                                                                                                        
        - ERC20Upgradeable.name() (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#68-70)                                
symbol() should be declared external:                                                                                                                      
        - ERC20Upgradeable.symbol() (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#76-78)                              
decimals() should be declared external:                                                                                                                    
        - ERC20Upgradeable.decimals() (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#93-95)                            
totalSupply() should be declared external:                                                                                                                 
        - ERC20Upgradeable.totalSupply() (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#100-102)                       
balanceOf(address) should be declared external:                                                                                                            
        - ERC20Upgradeable.balanceOf(address) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#107-109)                  
transfer(address,uint256) should be declared external:                                                                                                     
        - ERC20Upgradeable.transfer(address,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#119-122)           
allowance(address,address) should be declared external:                                                                                                    
        - ERC20Upgradeable.allowance(address,address) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#127-129)          
approve(address,uint256) should be declared external:                                                                                                      
        - ERC20Upgradeable.approve(address,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#138-141)            
transferFrom(address,address,uint256) should be declared external:                                                                                         
        - ERC20Upgradeable.transferFrom(address,address,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#156-170)                                                                                                                                                          
increaseAllowance(address,uint256) should be declared external:                                                                                            
        - ERC20Upgradeable.increaseAllowance(address,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#184-187)  
decreaseAllowance(address,uint256) should be declared external:                                                                                            
        - ERC20Upgradeable.decreaseAllowance(address,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#203-211)  
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-external 

Tools Used

Slither

Recommended Mitigation Steps

All of the public functions in the contract are not called internally, so access can be changed to external to reduce gas.

C4-008 : Redundant Code

Impact - Gas Optimization

Redundant code increase contract size and gas usage at deployment.

Proof of Concept

1. Navigate to the following contract inherited library lines.

AddressUpgradeable.functionCall(address,bytes) (lib/openzeppelin-contracts-upgradeable/contracts/utils/AddressUpgradeable.sol#80-82) is never used and should be removed                                                                                                                                              
AddressUpgradeable.functionCallWithValue(address,bytes,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/utils/AddressUpgradeable.sol#109-115) is never used and should be removed                                                                                                                           
AddressUpgradeable.functionStaticCall(address,bytes) (lib/openzeppelin-contracts-upgradeable/contracts/utils/AddressUpgradeable.sol#142-144) is never used and should be removed                                                                                                                                      
AddressUpgradeable.functionStaticCall(address,bytes,string) (lib/openzeppelin-contracts-upgradeable/contracts/utils/AddressUpgradeable.sol#152-161) is never used and should be removed                                                                                                                               
AddressUpgradeable.sendValue(address,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/utils/AddressUpgradeable.sol#55-60) is never used and should be removed                                                                                                                                               
ContextUpgradeable.__Context_init() (lib/openzeppelin-contracts-upgradeable/contracts/utils/ContextUpgradeable.sol#18-20) is never used and should be removed                                                                                                                                                         
ContextUpgradeable._msgData() (lib/openzeppelin-contracts-upgradeable/contracts/utils/ContextUpgradeable.sol#28-30) is never used and should be removed    
ERC20Upgradeable.__ERC20_init(string,string) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#55-58) is never used and should be removed                                                                                                                                            
ERC20Upgradeable.__ERC20_init_unchained(string,string) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#60-63) is never used and should be removed                                                                                                                                  
ERC20Upgradeable._burn(address,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#281-296) is never used and should be removed                                                                                                                                               
ERC20Upgradeable._mint(address,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#258-268) is never used and should be removed                                                                                                                                               
SafeERC20Upgradeable.safeApprove(IERC20Upgradeable,address,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/utils/SafeERC20Upgradeable.sol#45-58) is never used and should be removed                                                                                                           
SafeERC20Upgradeable.safeDecreaseAllowance(IERC20Upgradeable,address,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/utils/SafeERC20Upgradeable.sol#69-80) is never used and should be removed                                                                                                 
SafeERC20Upgradeable.safeIncreaseAllowance(IERC20Upgradeable,address,uint256) (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/utils/SafeERC20Upgradeable.sol#60-67) is never used and should be removed                                                                                                 
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#dead-code               
PausableUpgradeable.__gap (lib/openzeppelin-contracts-upgradeable/contracts/security/PausableUpgradeable.sol#97) is never used in TokenSaleUpgradeable (contracts/TokenSaleUpgradeable.sol#15-384)                                                                                                                    
ERC20Upgradeable.__gap (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#362) is never used in ERC20Upgradeable (lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/ERC20Upgradeable.sol#36-363)                                                                           
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unused-state-variable   

Tools Used

Slither

Recommended Mitigation Steps

Remove unused state variables.