Trader Joe contest - cmichel's results

Handle

cmichel

Vulnerability details

Imagine the following sequence of events:

• LaunchEvent.createPair() is called which sets wavaxReserve = 0, adds liquidity to the pair and receives lpSupply LP tokens.
• LaunchEvent.allowEmergencyWithdraw() is called which enters emergency / paused mode and disallows normal withdrawals.
• Users can only call LaunchEvent.emergencyWithdraw which reverts as the WAVAX reserve was already used to provide liquidity and cannot be paid out. Users don't receive their LP tokens either. The users lost their entire deposit in this case.

Recommendation

Consider paying out LP tokens in emergencyWithdraw.

Handle

cmichel

Vulnerability details

In LaunchEvent.createPair, when the floor price is not reached (floorPrice > wavaxReserve * 1e18 / tokenAllocated), the tokens to be sent to the pool are lowered to match the raised WAVAX at the floor price.

Note that the floorPrice is supposed to have a precision of 18:

/// @param _floorPrice Price of each token in AVAX, scaled to 1e18

The floorPrice > (wavaxReserve * 1e18) / tokenAllocated check is correct but the tokenAllocated computation involves the token decimals:

// @audit should be wavaxReserve * 1e18 / floorPrice
tokenAllocated = (wavaxReserve * 10**token.decimals()) / floorPrice;

This computation does not work for tokens that don't have 18 decimals.

Example

Assume I want to sell 1.0 wBTC = 1e8 wBTC (8 decimals) at 2,000.0 AVAX = 2,000 * 1e18 AVAX. The floorPrice is 2000e18 * 1e18 / 1e8 = 2e31

Assume the Launch event only raised 1,000.0 AVAX - half of the floor price for the issued token amount of 1.0 WBTC (it should therefore allocate only half a WBTC) - and the token amount will be reduced as: floorPrice = 2e31 > 1000e18 * 1e18 / 1e8 = 1e31 = actualPrice. Then, tokenAllocated = 1000e18 * 1e8 / 2e31 = 1e29 / 2e31 = 0 and no tokens would be allocated, instead of 0.5 WBTC = 0.5e8 WBTC.

The computation should be tokenAllocated = wavaxReserve * 1e18 / floorPrice = 1000e18 * 1e18 / 2e31 = 1e39 / 2e31 = 10e38 / 2e31 = 5e7 = 0.5e8.

Recommendation

The new tokenAllocated computation should be tokenAllocated = wavaxReserve * 1e18 / floorPrice;.

Handle

cmichel

Vulnerability details

The RocketJoeStaking.lastRewardTimestamp is initialized to zero. Usually, this does not matter as updatePool is called before the first deposit and when joeSupply = joe.balanceOf(address(this)) == 0, it is set to the current time.

function updatePool() public {
    if (block.timestamp <= lastRewardTimestamp) {
        return;
    }
    uint256 joeSupply = joe.balanceOf(address(this));

    // @audit lastRewardTimestamp is not initialized. can send 1 Joe to this contract directly => lots of rJoe minted to this contract
    if (joeSupply == 0) {
        lastRewardTimestamp = block.timestamp;
        return;
    }
    uint256 multiplier = block.timestamp - lastRewardTimestamp;
    uint256 rJoeReward = multiplier * rJoePerSec;
    accRJoePerShare =
        accRJoePerShare +
        (rJoeReward * PRECISION) /
        joeSupply;
    lastRewardTimestamp = block.timestamp;

    rJoe.mint(address(this), rJoeReward);
}

However, if a user first directly transfers Joe tokens to the contract before the first updatePool call, the block.timestamp - lastRewardTimestamp = block.timestamp will be a large timestamp value and lots of rJoe will be minted (but not distributed to users). Even though they are not distributed to the users, inflating the rJoe total supply might not be desired.

Recommendation

Consider tracking the actual total deposits in a storage variable and using this value instead of the current balance for joeSupply. This way, transferring tokens to the contract has no influence and depositing through deposit first calls updatePool and initializes lastRewardTimestamp.

Handle

cmichel

Vulnerability details

The LaunchEvent.createPair requires that no previous pool was created for the WAVAX <> _token pair.

function createPair() external isStopped(false) atPhase(Phase.PhaseThree) {
    (address wavaxAddress, address tokenAddress) = (
        address(WAVAX),
        address(token)
    );
    // @audit grief: anyone can create pair
    require(
        factory.getPair(wavaxAddress, tokenAddress) == address(0),
        "LaunchEvent: pair already created"
    );

    // ...
}

A griefer can create a pool for the WAVAX <> _token pair by calling JoeFactory.createPair(WAVAX, _token) while the launch event phase 1 or 2 is running. No liquidity can then be provided and an emergency state must be triggered for users and the issuer to be able to withdraw again.

Recommendation

It must be assumed that the pool is already created and even initialized as pool creation and liquidity provisioning is permissionless. Special attention must be paid if the pool is already initialized with liquidity at a different price than the launch event price.

It would be enough to have a standard min. LP return "slippage" check (using parameter values for amountAMin/amountBMin instead of the hardcoded ones in router.addLiquidity) in LaunchEvent.createPair(). The function must then be callable with special privileges only, for example, by the issuer. Alternatively, the slippage check can be hardcoded as a percentage of the raised amounts (amountADesired = 0.95 * wavaxReserve, amountBDesired = 0.95 * tokenAllocated).

This will prevent attacks that try to provide LP at a bad pool price as the transaction will revert when receiving less than the slippage parameter. If the pool is already initialized, it should just get arbitraged to the auction token price and liquidity can then be provided at the expected rate again.

Handle

cmichel

Vulnerability details

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead. Tokens that don't actually perform the transfer and return false are still counted as a correct transfer.

Recommendation

As the Launch event token can be any token, all interactions with it should follow correct EIP20 checks. We recommend checking the success boolean of all .transfer and .transferFrom calls for the unknown token contract.

• LaunchEvent.withdrawLiquidity: token.transfer(msg.sender, amount);
• LaunchEvent.withdrawIncentives: token.transfer(msg.sender, amount);
• LaunchEvent.emergencyWithdraw: token.transfer(msg.sender, amount);
• LaunchEvent.skim: token.transfer(msg.sender, amount);
• RocketJoeFactory.createRJLaunchEvent: IERC20(_token).transferFrom(msg.sender, launchEvent, _tokenAmount);