Badger Citadel contest - robee's results

Handle

robee

Vulnerability details

Title: Require with empty message Severity: Low Risk

The following requires are with empty messages. This is very important to add a message for any require. Such that the user has enough information to know the reason of failure:

    Solidity file: VipGuestListUpgradeable.sol, In line 60 with Empty Require message.
    Solidity file: VipGuestListUpgradeable.sol, In line 116 with Empty Require message.

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    TokenSaleUpgradeable.sol.initialize _tokenOut
    ForceEther.sol.forceSend recipient
    TokenSaleUpgradeable.sol.setSaleRecipient _saleRecipient
    VipGuestListUpgradeable.sol._verifyInvitationProof account
    VipGuestListUpgradeable.sol.proveInvitation account
    VipGuestListUpgradeable.sol.authorized _guest
    TokenSaleUpgradeable.sol.sweep _token
    TokenSaleUpgradeable.sol.setGuestlist _guestlist
    TokenSaleUpgradeable.sol.initialize _guestlist
    TokenSaleUpgradeable.sol.initialize _saleRecipient
    TokenSaleUpgradeable.sol.initialize _tokenIn

Title: Init frontrun Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    VipGuestListUpgradeable.sol, initialize, 34
    TokenSaleUpgradeable.sol, initialize, 96

Title: Never used parameters Severity: Low Risk

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

    MockERC20.sol: function constructor parameter _name isn't used. (constructor is default)
    MockERC20.sol: function constructor parameter _symbol isn't used. (constructor is default)

Title: Open TODOs Severity: Low Risk

Open TODOs can hint at programming or architectural errors that still need to be fixed. These files has open TODOs:

   Open TODO in BadgerGuestlistApi.sol line 3 : // TODO: Maybe rename

   Open TODO in TokenSaleUpgradeable.sol line 12 :  * TODO: Better revert strings

Handle

robee

Vulnerability details

Title: State variables that could be set immutable Severity: GAS

In the following files there are state variables that could be set immutable to save gas.

    tokenIn in TokenSaleUpgradeable.sol
    tokenOut in TokenSaleUpgradeable.sol

Title: Caching array length can save gas Severity: GAS

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }


    VipGuestListUpgradeable.sol, _guests, 117

Title: Prefix increments are cheaper than postfix increments Severity: GAS

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: VipGuestListUpgradeable.sol, i, 117

Title: Unnecessary index init Severity: GAS

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    VipGuestListUpgradeable.sol, 117

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    VipGuestListUpgradeable.sol, _setGuests
    VipGuestListUpgradeable.sol, _verifyInvitationProof

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    VipGuestListUpgradeable.sol, initialize
    VipGuestListUpgradeable.sol, proveInvitation
    MockERC20.sol, decimals
    TokenSaleUpgradeable.sol, saleEnded

Title: Short the following require messages Severity: GAS

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

    Solidity file: TokenSaleUpgradeable.sol, In line 114, Require message length to shorten: 37, The message: TokenSale: the price must not be zero
    Solidity file: TokenSaleUpgradeable.sol, In line 304, Require message length to shorten: 37, The message: TokenSale: the price must not be zero

Title: Use != 0 instead of > 0 Severity: GAS

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

    TokenSaleUpgradeable.sol, 289: change 'saleDuration > 0' to 'saleDuration != 0'
    TokenSaleUpgradeable.sol, 304: change '_tokenOutPrice > 0' to '_tokenOutPrice != 0'
    TokenSaleUpgradeable.sol, 114: change 'tokenOutPrice > 0' to 'tokenOutPrice != 0'
    TokenSaleUpgradeable.sol, 154: change '_tokenInAmount > 0' to '_tokenInAmount != 0'
    TokenSaleUpgradeable.sol, 289: change '_saleDuration > 0' to '_saleDuration != 0'
    TokenSaleUpgradeable.sol, 114: change '_tokenOutPrice > 0' to '_tokenOutPrice != 0'
    TokenSaleUpgradeable.sol, 111: change '_saleDuration > 0' to '_saleDuration != 0'
    TokenSaleUpgradeable.sol, 304: change 'tokenOutPrice > 0' to 'tokenOutPrice != 0'
    TokenSaleUpgradeable.sol, 258: change 'balance > 0' to 'balance != 0'
    TokenSaleUpgradeable.sol, 111: change 'saleDuration > 0' to 'saleDuration != 0'

Title: Use unchecked to save gas for certain additive calculations that cannot overflow Severity: GAS

You can use unchecked in the following calculations since there is no risk to overflow:

    TokenSaleUpgradeable.sol (L#241) - hasEnded_ = (block.timestamp >= saleStart + saleDuration) ||
    TokenSaleUpgradeable.sol (L#150) - require( block.timestamp < saleStart + saleDuration, "TokenSale: already ended" );

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    MockERC20.constructor (_name)
    MockERC20.constructor (_symbol)

Title: Check if amount is not zero to save gas Severity: GAS

The following functions could skip other steps if the amount is 0. (A similar issue: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/82)

    TokenSaleUpgradeable.sol, getAmountOut