Trader Joe contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

Some functions could be strengthened by adding re-entrancy protection: e.g. createRJLaunchEvent does not follow the Checks-Effects-Interactions pattern: first, it transfers the tokens from the sender and then initializes the event:

  IERC20(_token).transferFrom(msg.sender, launchEvent, _tokenAmount);
  ILaunchEvent(payable(launchEvent)).initialize(
      _issuer,
      _phaseOneStartTime,
      _token,
      _tokenIncentivesPercent,
      _floorPrice,
      _maxWithdrawPenalty,
      _fixedWithdrawPenalty,
      _maxAllocation,
      _userTimelock,
      _issuerTimelock
  );

and only then sets the state:

  getRJLaunchEvent[_token] = launchEvent;
  isRJLaunchEvent[launchEvent] = true;
  allRJLaunchEvents.push(launchEvent);

Because there is no whitelist, and the function accepts any token, if this token contains a transfer callback hook, it could be exploited to initialize the launch event for the same token several times, bypassing the previous checks, e.g.:

  require(
      getRJLaunchEvent[_token] == address(0),
      "RJFactory: token has already been issued"
  );

_safeTransferAVAX function is also susceptible to re-entrancy, but I didn't find an exact attack path to cause any significant harm.

Recommended Mitigation Steps

I suggest revisiting all the functions and applying for re-entrancy protection where external calls are performed and the Checks-Effects-Interactions pattern is not followed.

Handle

pauliax

Vulnerability details

Impact

This is an issue as well as a gas improvment. Create pair adjusts token amounts based on floor price:

    if (
        floorPrice > (wavaxReserve * 10**token.decimals()) / tokenAllocated
    ) {
        tokenAllocated = (wavaxReserve * 10**token.decimals()) / floorPrice;

The problem is that .decimals() is not part of the official ERC20 standard and might fail for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a valid concern, especially considering that createPair can only be called once after phase 3 has started.

Recommended Mitigation Steps

The current approach may be useful in case the token can change its decimals (usually very unlikely but possible), but I think a better solution would be to initialize the decimals in the initialize function or at least make a test call to see if this token is compatible. Another possibility is to write a helper function, e.g. similar to the popular safeTransfer.

And finally, a gas improvement here would be to avoid this duplicate external call and multiplication: (wavaxReserve * 10**token.decimals()) You should cache the result and re-use it inside the if statement.

Handle

pauliax

Vulnerability details

Impact

Since not all ERC-20 tokens adhere to the standard, it is recommended to use SafeERC20 functions such that interactions with a broader range of tokens are possible. Especially, this is important since interactions with some tokens, e.g. USDT, require safeApprove.

  IERC20(_token).transferFrom(msg.sender, launchEvent, _tokenAmount);

Similarly with approve -> safeApprove:

  token.approve(address(router), tokenAllocated);

Recommended Mitigation Steps

Consider using https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

Handle

pauliax

Vulnerability details

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. e.g.:

  require(
        _startTime > block.timestamp,
        "RocketJoeStaking: rJOE minting needs to start after the current timestamp"
    );
  require(
      balance > 0,
      "LaunchEvent: expected user to have non-zero balance to perform emergency withdraw"
  );
  require(
      msg.sender == address(WAVAX),
      "LaunchEvent: you can't send AVAX directly to this contract"
  );

Handle

pauliax

Vulnerability details

Impact

I don't see a reason why function _atPhase can't be replaced with just this:

  require(
    currentPhase() == _phase,
    "LaunchEvent: wrong phase"
  );

Handle

pauliax

Vulnerability details

Impact

phase3Start will not change once auctionStart is initialized, so I think it would make sense to calculate it once and store the value instead of re-calculating it every time:

    modifier timelockElapsed() {
        uint256 phase3Start = auctionStart +
            PHASE_ONE_DURATION +
            PHASE_TWO_DURATION;

Handle

pauliax

Vulnerability details

Impact

I think it would make sense to separate issuer functions from regular users, as usually it contains different logics anyway, e.g. function withdrawLiquidity():

    if (msg.sender == issuer) {
        balance = lpSupply / 2;

        emit IssuerLiquidityWithdrawn(msg.sender, address(pair), balance);

        if (tokenReserve > 0) {
            uint256 amount = tokenReserve;
            tokenReserve = 0;
            token.transfer(msg.sender, amount);
        }
    } else {
        emit UserLiquidityWithdrawn(msg.sender, address(pair), balance);
    }

The check that the msg.sender is issuer will be irrelevant >99% of the time (if we assume there will be many users), but it will consume extra gas every time. Also, it would help to keep a single responsibility principle. But the deployment costs might be a bit higher.

Handle

pauliax

Vulnerability details

Impact

The unchecked keyword can be applied in the following lines of code since there are statements before to ensure the arithmetic operations would not cause an integer underflow or overflow:

    if (newAllocation > user.allocation) {
        ...
        rJoeNeeded = getRJoeAmount(newAllocation - user.allocation);


      if (block.timestamp > lastRewardTimestamp && joeSupply != 0) {
            uint256 multiplier = block.timestamp - lastRewardTimestamp;


        require(
            user.amount >= _amount,
            "RocketJoeStaking: withdraw amount exceeds balance"
        );
       ...
        user.amount = user.amount - _amount;


        if (block.timestamp <= lastRewardTimestamp) {
            return;
        }
        ...
        uint256 multiplier = block.timestamp - lastRewardTimestamp;

Handle

pauliax

Vulnerability details

Impact

The unchecked keyword can be applied in the following lines of code since there are statements before to ensure the arithmetic operations would not cause an integer underflow or overflow:

    if (newAllocation > user.allocation) {
        ...
        rJoeNeeded = getRJoeAmount(newAllocation - user.allocation);


      if (block.timestamp > lastRewardTimestamp && joeSupply != 0) {
            uint256 multiplier = block.timestamp - lastRewardTimestamp;


        require(
            user.amount >= _amount,
            "RocketJoeStaking: withdraw amount exceeds balance"
        );
       ...
        user.amount = user.amount - _amount;


        if (block.timestamp <= lastRewardTimestamp) {
            return;
        }
        ...
        uint256 multiplier = block.timestamp - lastRewardTimestamp;

Handle

pauliax

Vulnerability details

Impact

Avoid repeated storage access:

  require(
      user.balance > 0,
      "LaunchEvent: expected user to have non-zero balance to perform emergency withdraw"
  );
  uint256 balance = user.balance;

Proposed improvement:

  uint256 balance = user.balance;
    require(
      balance > 0,
      "LaunchEvent: expected user to have non-zero balance to perform emergency withdraw"
  );

Handle

pauliax

Vulnerability details

Impact

Anyone can initialize a new rJOE token. The first time rJoe token is set in the constructor of the contract RocketJoeFactory, but there is also a function called setRJoe to change this address later:

  function setRJoe(address _rJoe) external override onlyOwner {
      IRocketJoeToken(_rJoe).initialize();
      rJoe = _rJoe;
      emit SetRJoe(_rJoe);
  }

The problem is when this new token is deployed and setRJoe is called in a separate transaction. This makes it vulnerable to the frontrunning attack:

1. owner deploys new rJoe token
2. owner calls setRJoe on RocketJoeFactory, but a malicious actor monitors the mempool and frontruns this tx by directly calling initialize() on this new token contract setting himself as the factory.

While I can't make assumptions that this will always be the case, based on the current implementation of RocketJoeToken, the initialize() function is unprotected, thus the first one to call it is assigned as a factory.

Recommended Mitigation Steps

Maybe later implementations will have an auth protection as the factory address will be known upfront, but I just wanted you to be aware of this potential attack, so you can actor accordingly and prevent a new rJoe token get hijacked by a malicious actor.