QA Report

[code423n4]

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: code-423n4/2021-09-sushimiso-findings#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