code-423n4 / 2022-01-trader-joe-findings

2 stars 0 forks source link

Re-entrancy protection #285

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

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.

cryptofish7 commented 2 years ago

Duplicate of #248