Trader Joe contest - defsec's results

Handle

defsec

Vulnerability details

Impact

During the code review, It has been observed that the token can be same as WAVAX. The initialize function should not allow if token is equals to wavax. That would affect all asset management.

Proof of Concept

1. Navigate to the following contract.

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L219

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L411

Tools Used

Code Review

Recommended Mitigation Steps

On the Launchevent, token should not be equal to wavax.

Handle

defsec

Vulnerability details

Impact

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-01-trader-joe/blob/main/contracts/RocketJoeStaking.sol#L57

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.

Handle

defsec

Vulnerability details

Impact

"RocketJoeToken.sol", "RocketJoeFactory" and "RocketJoeStaking" inherit OpenZeppelin's Ownable 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-01-trader-joe/blob/main/contracts/RocketJoeToken.sol#L12", "https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L18" and "https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeStaking.sol#L14"
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.

Handle

defsec

Vulnerability details

Impact

Some tokens (like USDT L199) 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.

IERC20(token).approve(address(operator), 0);
IERC20(token).approve(address(operator), amount);

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-01-trader-joe/blob/a1579f6453bc4bf9fb0db9c627beaa41135438ed/contracts/LaunchEvent.sol#L408

2. When trying to re-approve an already approved token, all transactions revert and the protocol cannot be used.

Tools Used

None

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

Handle

defsec

Vulnerability details

Impact

The following contract functions performs an ERC20.approve() call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-01-trader-joe/blob/a1579f6453bc4bf9fb0db9c627beaa41135438ed/contracts/LaunchEvent.sol#L408

2. Tokens that don't actually perform the approve and return false are still counted as a correct approve.

Tools Used

None

Recommended Mitigation Steps

Its recommend to using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.

Reference : https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.1/contracts/token/ERC20/utils/SafeERC20.sol#L74

Handle

defsec

Vulnerability details

Impact

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 contracts.

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L158

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L171

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L178

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L185

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L192

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L218

Tools Used

Code Review

Recommended Mitigation Steps

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

Handle

defsec

Vulnerability details

Vulnerability details

Impact

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

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L338

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L355

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L370

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L390

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L486

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L498

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeStaking.sol#L101

Tools Used

Code Review

Recommended Mitigation Steps

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

Handle

defsec

Vulnerability details

Impact

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

Revert strings > 32 bytes are here:

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L311

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L621

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L224

Tools Used

Manual Review

Recommended Mitigation Steps

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

Handle

defsec

Vulnerability details

Impact

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

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeStaking.sol#L167

https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeStaking.sol#L87

Tools Used

None

Recommended Mitigation Steps

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

Handle

defsec

Vulnerability details

Impact

In the JoeStaking contract, the amount check should be placed on the contract. IF the amount is more than transfer operations should be completed.

Proof of Concept

1. Navigate to the following contract.

https://github.com/code-423n4/2022-01-trader-joe/blob/a1579f6453bc4bf9fb0db9c627beaa41135438ed/contracts/RocketJoeStaking.sol#L129

2. _amount is not checked if Its more than zero.

    function withdraw(uint256 _amount) external {
        UserInfo storage user = userInfo[msg.sender];
        require(
            user.amount >= _amount,
            "RocketJoeStaking: withdraw amount exceeds balance"
        );

        updatePool();

        uint256 pending = (user.amount * accRJoePerShare) /
            PRECISION -
            user.rewardDebt;

        user.amount = user.amount - _amount;
        user.rewardDebt = (user.amount * accRJoePerShare) / PRECISION;

        _safeRJoeTransfer(msg.sender, pending);
        joe.safeTransfer(address(msg.sender), _amount);
        emit Withdraw(msg.sender, _amount);
    }

Tools Used

Code Review

Recommended Mitigation Steps

Consider to add the following check.

    function withdraw(uint256 _amount) external {
        UserInfo storage user = userInfo[msg.sender];
        require(
            user.amount >= _amount,
            "RocketJoeStaking: withdraw amount exceeds balance"
        );

        updatePool();

        uint256 pending = (user.amount * accRJoePerShare) /
            PRECISION -
            user.rewardDebt;

        user.amount = user.amount - _amount;
        user.rewardDebt = (user.amount * accRJoePerShare) / PRECISION;

 if(pending != 0) {
        _safeRJoeTransfer(msg.sender, pending);
}

if(_amount != 0){
        joe.safeTransfer(address(msg.sender), _amount);
}

        emit Withdraw(msg.sender, _amount);

    }

Handle

defsec

Vulnerability details

Impact

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

• Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
• Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
• Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
• Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Proof of Concept

1. The contest repository contracts contain floating pragma ^0.8.0. The contracts pragma version can be updated to 0.8.4 for the gas optimization.

• Affected Contracts

All Contracts

Tools Used

None

Recommended Mitigation Steps

Consider to upgrade pragma to at least 0.8.4.