Trader Joe contest - robee's results

Handle

robee

Vulnerability details

Not verified address arguments of external/public functions is a low risk issue. It's less severe for onlyOwner methods but for any other method it's crucial since the default address is 0.

    Argument tokenA of IJoeFactory.sol.getPair is not verified to be != 0
    Argument tokenB of IJoeFactory.sol.getPair is not verified to be != 0
    Argument tokenA of IJoeFactory.sol.createPair is not verified to be != 0
    Argument tokenB of IJoeFactory.sol.createPair is not verified to be != 0
    Argument None of IJoeFactory.sol.setFeeTo is not verified to be != 0

Handle

robee

Vulnerability details

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.

The 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:

    RocketJoeStaking.sol, initialize, 57

Handle

robee

Vulnerability details

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value.They must first be approved by zero and then the actual allowance must be approved. You don't first approve 0 in the following places in the codebase:

    approve without approving 0 first LaunchEvent.sol, 407, token.approve(address(router), tokenAllocated); 

Handle

robee

Vulnerability details

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

    Instead a < b / c use a * c < b. 

In all of the big and trusted contracts this rule is maintained (for example look at AAVE codebase).

    LaunchEvent.sol, 395, if ( floorPrice > (wavaxReserve * 10**token.decimals()) / tokenAllocated ) {

Handle

robee

Vulnerability details

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: LaunchEvent.sol, In line 622, Require message length to shorten: 33, The message: LaunchEvent: avax transfer failed
    Solidity file: RocketJoeToken.sol, In line 26, Require message length to shorten: 35, The message: RocketJoeToken: already initialized
    Solidity file: RocketJoeFactory.sol, In line 116, Require message length to shorten: 35, The message: RJFactory: token can't be 0 address
    Solidity file: RocketJoeFactory.sol, In line 115, Require message length to shorten: 36, The message: RJFactory: issuer can't be 0 address

Handle

robee

Vulnerability details

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

    LaunchEvent.sol, 355: change '_amount > 0' to '_amount != 0'
    LaunchEvent.sol, 358: change 'balance > 0' to 'balance != 0'
    LaunchEvent.sol, 390: change 'wavaxReserve > 0' to 'wavaxReserve != 0'
    LaunchEvent.sol, 390: change 'balance > 0' to 'balance != 0'
    LaunchEvent.sol, 455: change 'tokenReserve > 0' to 'tokenReserve != 0'
    LaunchEvent.sol, 498: change 'balance > 0' to 'balance != 0'
    RocketJoeFactory.sol, 119: change '_tokenAmount > 0' to '_tokenAmount != 0'

Handle

robee

Vulnerability details

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

    LaunchEvent.sol, withdrawAVAX
    ERC20Token6decimals.sol, decimals
    RocketJoeStaking.sol, initialize

Handle

robee

Vulnerability details

You use safeApprove of openZeppelin although it's deprecated. (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38) You should change it to increase/decrease Allowance as OpenZeppilin says. This appears in the following locations in the code base:

    Deprecated safeApprove in LaunchEvent.sol line 406: WAVAX.approve(address(router), wavaxReserve); 
    Deprecated safeApprove in LaunchEvent.sol line 407: token.approve(address(router), tokenAllocated); 

Handle

robee

Vulnerability details

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the overall gas uses. The following is a list of functions and the storage variables that you read twice:

    LaunchEvent.sol: issuer is read twice in depositAVAX
    LaunchEvent.sol: auctionStart is read twice in currentPhase
    LaunchEvent.sol: wavaxReserve is read twice in createPair

Handle

robee

Vulnerability details

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)

    LaunchEvent.sol, receive
    LaunchEvent.sol, initialize
    LaunchEvent.sol, currentPhase
    LaunchEvent.sol, depositAVAX
    LaunchEvent.sol, createPair
    LaunchEvent.sol, withdrawLiquidity
    LaunchEvent.sol, withdrawIncentives

Handle

robee

Vulnerability details

You calculate the power of 10 every time you use it instead of caching it once as a constant variable and using it instead. Fix the following code lines:

    LaunchEvent.sol, 395 : You should cache the used power of 10 as constant state variable since it's used several times (2): floorPrice > (wavaxReserve * 10**token.decimals()) / tokenAllocated 
    LaunchEvent.sol, 397 : You should cache the used power of 10 as constant state variable since it's used several times (2): tokenAllocated = (wavaxReserve * 10**token.decimals()) / floorPrice; 

Handle

robee

Vulnerability details

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

    RocketJoeToken.sol, _beforeTokenTransfer

This is the only one so it's important for quality.