Platform: Code4rena
Start Date: 25/01/2022
Pot Size: $50,000 USDT
Total HM: 17
Participants: 39
Period: 3 days
Judge: LSDan
Total Solo HM: 9
Id: 79
League: ETH
Rank: 24/39
Findings: 3
Award: $247.29
🌟 Selected for report: 4
🚀 Solo Findings: 0
Czar102
ERC20 tokens may not revert on failure, instead return false. Users may lose their funds as ERC20 calls may fail. They are made to unknown ERC20 tokens that have no additional constraints on their failure reporting.
LaunchEvent.sol::458 => token.transfer(msg.sender, amount); LaunchEvent.sol::464 => pair.transfer(msg.sender, balance); LaunchEvent.sol::490 => token.transfer(msg.sender, amount); LaunchEvent.sol::514 => token.transfer(issuer, balance); LaunchEvent.sol::538 => token.transfer(penaltyCollector, excessToken); LaunchEvent.sol::543 => WAVAX.transfer(penaltyCollector, excessWavax); RocketJoeFactory.sol::133 => IERC20(_token).transferFrom(msg.sender, launchEvent, _tokenAmount); RocketJoeStaking.sol::184 => rJoe.transfer(_to, rJoeBal); RocketJoeStaking.sol::186 => rJoe.transfer(_to, _amount);
Manual analysis c4udit
#0 - cryptofish7
2022-02-10T13:55:15Z
Duplicate of #232
#1 - dmvt
2022-02-22T19:25:48Z
duplicate of #198
Czar102
A bad actor can call the initialize() function of RocketJoeToken right after deployment, so that it will be useless to the deployer. A deployer might have already breadcasted a transaction deploying RocketJoeFactory and it will fail as the constructor requires RocketJoeToken's initialize() call not to revert.
function initialize() external { require( address(rocketJoeFactory) == address(0), "RocketJoeToken: already initialized" ); rocketJoeFactory = IRocketJoeFactory(msg.sender); }
Manual analysis
Make owner set the rocketJoeFactory variable once, after RocketJoeFactory is deployed.
#0 - cryptofish7
2022-02-10T20:22:29Z
Duplicate of #8
#1 - dmvt
2022-02-22T10:42:58Z
Assets are not at risk. The worst case is that there is lost gas and the contracts need to be redeployed. Consider creating these contracts and calling their initialize functions in a factory to mitigate the issue if not adding the guard.
1 — Low (L): vulns that have a risk of 1 are considered “Low” severity when assets are not at risk. Includes state handling, function incorrect as to spec, and issues with comments.
Czar102
Issue Information: G003
LaunchEvent.sol::314 => msg.value > 0, LaunchEvent.sol::338 => if (rJoeNeeded > 0) { LaunchEvent.sol::355 => require(_amount > 0, "LaunchEvent: invalid withdraw amount"); LaunchEvent.sol::370 => if (feeAmount > 0) { LaunchEvent.sol::390 => require(wavaxReserve > 0, "LaunchEvent: no wavax balance"); LaunchEvent.sol::455 => if (tokenReserve > 0) { LaunchEvent.sol::486 => require(amount > 0, "LaunchEvent: caller has no incentive to claim"); LaunchEvent.sol::498 => user.balance > 0, LaunchEvent.sol::537 => if (excessToken > 0) { LaunchEvent.sol::542 => if (excessWavax > 0) { LaunchEvent.sol::547 => if (excessAvax > 0) _safeTransferAVAX(penaltyCollector, excessAvax); RocketJoeFactory.sol::119 => _tokenAmount > 0, RocketJoeStaking.sol::101 => if (user.amount > 0) {
#0 - cryptofish7
2022-02-10T13:58:38Z
Duplicate of #240
🌟 Selected for report: WatchPug
Also found by: Czar102, Dravee, Jujic, Meta0xNull, byterocket, defsec, p4st13r4, pauliax, robee, sirhashalot
Czar102
Issue Information: G007
LaunchEvent.sol::165 => "LaunchEvent: you can't send AVAX directly to this contract" LaunchEvent.sol::183 => "LaunchEvent: can't withdraw before issuer's timelock" LaunchEvent.sol::188 => "LaunchEvent: can't withdraw before user's timelock" LaunchEvent.sol::239 => "LaunchEvent: maxWithdrawPenalty too big" LaunchEvent.sol::243 => "LaunchEvent: fixedWithdrawPenalty too big" LaunchEvent.sol::247 => "LaunchEvent: can't lock user LP for more than 7 days" LaunchEvent.sol::251 => "LaunchEvent: issuer can't withdraw before users" LaunchEvent.sol::255 => "LaunchEvent: start of phase 1 cannot be in the past" LaunchEvent.sol::312 => require(msg.sender != issuer, "LaunchEvent: issuer cannot participate"); LaunchEvent.sol::315 => "LaunchEvent: expected non-zero AVAX to deposit" LaunchEvent.sol::322 => "LaunchEvent: amount exceeds max allocation" LaunchEvent.sol::355 => require(_amount > 0, "LaunchEvent: invalid withdraw amount"); LaunchEvent.sol::359 => "LaunchEvent: withdrawn amount exceeds balance" LaunchEvent.sol::388 => "LaunchEvent: liquid pair already exists" LaunchEvent.sol::444 => "LaunchEvent: liquidity already withdrawn" LaunchEvent.sol::474 => "LaunchEvent: incentives already withdrawn" LaunchEvent.sol::486 => require(amount > 0, "LaunchEvent: caller has no incentive to claim"); LaunchEvent.sol::499 => "LaunchEvent: expected user to have non-zero balance to perform emergency withdraw" LaunchEvent.sol::523 => "LaunchEvent: caller is not RocketJoeFactory owner" LaunchEvent.sol::622 => require(success, "LaunchEvent: avax transfer failed"); RocketJoeFactory.sol::60 => "RJFactory: Addresses can't be null address" RocketJoeFactory.sol::113 => "RJFactory: token has already been issued" RocketJoeFactory.sol::115 => require(_issuer != address(0), "RJFactory: issuer can't be 0 address"); RocketJoeFactory.sol::116 => require(_token != address(0), "RJFactory: token can't be 0 address"); RocketJoeFactory.sol::120 => "RJFactory: token amount needs to be greater than 0" RocketJoeFactory.sol::127 => "RJFactory: liquid pair already exists" RocketJoeFactory.sol::208 => "RJFactory: phase one duration lower than no fee duration" RocketJoeFactory.sol::225 => "RJFactory: no fee duration bigger than phase one duration" RocketJoeStaking.sol::67 => "RocketJoeStaking: rJOE minting needs to start after the current timestamp" RocketJoeStaking.sol::120 => "RocketJoeStaking: withdraw amount exceeds balance" RocketJoeToken.sol::19 => "RocketJoeToken: caller is not a RJLaunchEvent" RocketJoeToken.sol::28 => "RocketJoeToken: already initialized"
#0 - cryptofish7
2022-02-10T13:57:12Z
Duplicate of #242
🌟 Selected for report: Czar102
Also found by: Dravee, byterocket, d4rk
7.2498 USDT - $7.25
🌟 Selected for report: kirk-baird
10.7404 USDT - $10.74
Czar102
_atPhase internal function of LaunchEvent uses many ifs to determine if the phase is matching the desired one.
function _atPhase(Phase _phase) internal view { if (_phase == Phase.NotStarted) { require( currentPhase() == Phase.NotStarted, "LaunchEvent: not in not started" ); } else if (_phase == Phase.PhaseOne) { require( currentPhase() == Phase.PhaseOne, "LaunchEvent: not in phase one" ); } else if (_phase == Phase.PhaseTwo) { require( currentPhase() == Phase.PhaseTwo, "LaunchEvent: not in phase two" ); } else if (_phase == Phase.PhaseThree) { require( currentPhase() == Phase.PhaseThree, "LaunchEvent: not in phase three" ); } else { revert("LaunchEvent: unknown state"); } }
Manual analysis
First check if current phase and _phase match, pass if yes.
If not, use switch-case syntax to determine the error thrown.
#0 - cryptofish7
2022-02-10T13:44:04Z
Duplicate of #162
🌟 Selected for report: Czar102
39.7792 USDT - $39.78
Czar102
_atPhase internal function of LaunchEvent contains a revert statement that will never be triggered as the argument of the function is always one of the correct states, based on all uses.
} else { revert("LaunchEvent: unknown state"); }
Manual analysis
Delete the else statement as it is redundant.
🌟 Selected for report: Czar102
39.7792 USDT - $39.78
Czar102
Usage of constructors allows immutable variables to be defined and can lower usage gas costs.
RocketJoeStaking contract contains joe and rJoe variables that could have been immutable.
Manual analysis
🌟 Selected for report: kirk-baird
Also found by: Czar102
17.9007 USDT - $17.90
Czar102
Storage variables are often read more than once, despite no change in their value.
function deposit(uint256 _amount) external { UserInfo storage user = userInfo[msg.sender]; updatePool(); if (user.amount > 0) { uint256 pending = (user.amount * accRJoePerShare) / PRECISION - user.rewardDebt; _safeRJoeTransfer(msg.sender, pending); } user.amount = user.amount + _amount; user.rewardDebt = (user.amount * accRJoePerShare) / PRECISION; joe.safeTransferFrom(address(msg.sender), address(this), _amount); emit Deposit(msg.sender, _amount); }
In the above example, user.amount is read 4 times, while it could have been read only once.
Manual analysis
Cache unchanging storage variables on the stack.
#0 - cryptofish7
2022-02-10T14:30:18Z
Duplicate of #128
🌟 Selected for report: Czar102
Czar102
Storage variable reads are gas-intensive, mark unchanging variables immutable in the case of non-upgradeable contracts.
RochetJoeStaking.sol#33: IERC20Upgradeable public joe; RochetJoeStaking.sol#41: RocketJoeToken public rJoe; RochetJoeFactory.sol#23: address public override rJoe; RochetJoeFactory.sol#25: address public override wavax;
Manual analysis
#0 - cryptofish7
2022-02-10T13:53:12Z
Duplicate of #62
#1 - dmvt
2022-02-21T12:58:17Z
This one is partially valid. RocketJoeStaking is proxied so you can't make the variables immutable. RocketJoeFactory rJoe can be set to a different value after the constructor is called.
RocketJoeFactory wavax, however, is only set during the constructor and is never changed. Using immutable here would save some gas.
Czar102
Issue Information: L003
LaunchEvent.sol::4 => pragma solidity ^0.8.0; RocketJoeFactory.sol::4 => pragma solidity ^0.8.0; RocketJoeStaking.sol::3 => pragma solidity ^0.8.0; RocketJoeToken.sol::3 => pragma solidity ^0.8.0;
#0 - cryptofish7
2022-02-10T13:54:10Z
Duplicate of #181
#1 - dmvt
2022-02-22T14:09:50Z
Typically I'd consider this a non-critical issue, but in this case I'm going to call it a gas issue given the savings available by locking in a higher version of solidity.