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: 13/39
Findings: 6
Award: $1,506.17
π Selected for report: 11
π Solo Findings: 0
700.6157 USDT - $700.62
WatchPug
function createRJLaunchEvent( address _issuer, uint256 _phaseOneStartTime, address _token, uint256 _tokenAmount, uint256 _tokenIncentivesPercent, uint256 _floorPrice, uint256 _maxWithdrawPenalty, uint256 _fixedWithdrawPenalty, uint256 _maxAllocation, uint256 _userTimelock, uint256 _issuerTimelock ) external override returns (address) { require( getRJLaunchEvent[_token] == address(0), "RJFactory: token has already been issued" ); require(_issuer != address(0), "RJFactory: issuer can't be 0 address"); require(_token != address(0), "RJFactory: token can't be 0 address"); require(_token != wavax, "RJFactory: token can't be wavax"); require( _tokenAmount > 0, "RJFactory: token amount needs to be greater than 0" ); require( IJoeFactory(factory).getPair(_token, wavax) == address(0) || IJoePair(IJoeFactory(factory).getPair(_token, wavax)) .totalSupply() == 0, "RJFactory: liquid pair already exists" ); address launchEvent = Clones.clone(eventImplementation); // msg.sender needs to approve RocketJoeFactory IERC20(_token).transferFrom(msg.sender, launchEvent, _tokenAmount);
In the current implementation, RocketJoeFactory.sol#createRJLaunchEvent() can be called by anyone with at least 1 Wei of _token.
This allows a malicious user or attacker to call createRJLaunchEvent() with minimal cost and stop others, especially the platform itself or the rightful issuer of the token from creating the RJLaunchEvent.
Consider making createRJLaunchEvent() only callable by the owner of RocketJoeFactory.
#0 - cryptofish7
2022-02-10T14:16:15Z
Thatβs the spirit, not a single token should be in circulation
π Selected for report: cccz
Also found by: 0x1f8b, Dravee, TomFrenchBlockchain, UncleGrandpa925, WatchPug, bobi, byterocket, hack3r-0m, sirhashalot
WatchPug
It is usually good to add a require-statement that checks the return value or to use something like safeTransfer; unless one is sure the given token reverts in case of a failure.
Instances include:
token.transfer(msg.sender, amount);
token.transfer(msg.sender, amount);
token.transfer(issuer, balance);
token.transfer(penaltyCollector, excessToken);
Consider adding a require-statement or using safeTransfer.
#0 - cryptofish7
2022-01-31T14:46:25Z
Duplicate of #12
#1 - dmvt
2022-02-22T10:50:44Z
This could result in a loss of funds given the right external conditions.
2 β Med (M): vulns have a risk of 2 and are considered βMediumβ severity when assets are not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
π Selected for report: cmichel
Also found by: UncleGrandpa925, WatchPug, harleythedog
283.7493 USDT - $283.75
WatchPug
function createPair() external isStopped(false) atPhase(Phase.PhaseThree) { (address wavaxAddress, address tokenAddress) = ( address(WAVAX), address(token) ); require( factory.getPair(wavaxAddress, tokenAddress) == address(0) || IJoePair( IJoeFactory(factory).getPair(wavaxAddress, tokenAddress) ).totalSupply() == 0, "LaunchEvent: liquid pair already exists" );
createPair() is supposed to be called after phase 3 has started to finalize the launch event and all users can withdrawLiquidity() after this.
However, since createPair() requires the wavaxAddress, tokenAddress pair not existing or the totalSupply == 0, if someone (can be an attacker, a malicious user, or just a regular user) add liquidity to the pair during phase 1 and 2, or phase 3 before createPair() is called, it will make the launch event can not be finalized, therefore, forcing the event to be canceled and put into emergencyWithdraw mode.
Consider allowing createPair() when the pair is at a certain price range, so that even in the case above, the launch event can still be finalized, by combining a swap tx to correct the price to the target range with the createPair() tx.
#0 - cryptofish7
2022-02-01T00:40:54Z
Fixed with low level mint https://github.com/traderjoe-xyz/rocket-joe/pull/81/files
#1 - dmvt
2022-02-22T19:49:59Z
duplicate of #197
74.4672 USDT - $74.47
WatchPug
It is usually good to add a require-statement that checks the return value or to use something like safeTransferFrom; unless one is sure the given token reverts in case of a failure.
/// ... /// @param _token Token that will be issued through this launch event /// ... function createRJLaunchEvent( // ... address _token, // ... ) external override returns (address) { // ... IERC20(_token).transferFrom(msg.sender, launchEvent, _tokenAmount); // ... }
Consider adding a require-statement or using safeTransferFrom.
#0 - cryptofish7
2022-01-31T22:29:31Z
#1 - dmvt
2022-02-22T19:25:19Z
duplicate of #198
233.5386 USDT - $233.54
WatchPug
function createRJLaunchEvent( address _issuer, uint256 _phaseOneStartTime, address _token, uint256 _tokenAmount, uint256 _tokenIncentivesPercent, uint256 _floorPrice, uint256 _maxWithdrawPenalty, uint256 _fixedWithdrawPenalty, uint256 _maxAllocation, uint256 _userTimelock, uint256 _issuerTimelock ) external override returns (address) { require( getRJLaunchEvent[_token] == address(0), "RJFactory: token has already been issued" ); require(_issuer != address(0), "RJFactory: issuer can't be 0 address"); require(_token != address(0), "RJFactory: token can't be 0 address"); require(_token != wavax, "RJFactory: token can't be wavax"); require( _tokenAmount > 0, "RJFactory: token amount needs to be greater than 0" ); require( IJoeFactory(factory).getPair(_token, wavax) == address(0) || IJoePair(IJoeFactory(factory).getPair(_token, wavax)) .totalSupply() == 0, "RJFactory: liquid pair already exists" ); address launchEvent = Clones.clone(eventImplementation); // msg.sender needs to approve RocketJoeFactory IERC20(_token).transferFrom(msg.sender, launchEvent, _tokenAmount); ILaunchEvent(payable(launchEvent)).initialize( _issuer, _phaseOneStartTime, _token, _tokenIncentivesPercent, _floorPrice, _maxWithdrawPenalty, _fixedWithdrawPenalty, _maxAllocation, _userTimelock, _issuerTimelock ); getRJLaunchEvent[_token] = launchEvent; isRJLaunchEvent[launchEvent] = true; allRJLaunchEvents.push(launchEvent); _emitLaunchedEvent(_issuer, _token, _phaseOneStartTime); return launchEvent; }
At L132, _token.transferFrom() can be used to re-enter the createRJLaunchEvent() function, before the storage change at L147-149.
This will allow the attacker to create multiple launchEvent contracts and get them listed in allRJLaunchEvents.
Even though there is no significant impact as far as we can tell from the smart contract code. We believe this is still unexpected and may cause other parts of the system, say the frontend to malfunction in some cases.
Consider moving L132 _token.transferFrom() to after L147-149 to prevent re-entrance.
#0 - cryptofish7
2022-01-31T22:34:33Z
WatchPug
There are many functions across the codebase that will perform 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.
Instances include:
WAVAX.approve(address(router), wavaxReserve); token.approve(address(router), tokenAllocated);
It is usually good to add a require-statement that checks the return value or to use something like SafeERC20#safeIncreaseAllowance(); unless one is sure the given token reverts in case of a failure.
#0 - cryptofish7
2022-01-31T14:30:51Z
Duplicate of #154
0.8642 USDT - $0.86
WatchPug
It is cheaper to use != 0 than > 0 for uint256.
if (user.amount > 0) {
_tokenAmount > 0,
if (rJoeNeeded > 0) {
require(_amount > 0, "LaunchEvent: invalid withdraw amount");
if (feeAmount > 0) {
if (tokenReserve > 0) {
if (excessToken > 0) {
#0 - cryptofish7
2022-01-31T20:09:39Z
WatchPug
address public override eventImplementation;
address public override wavax;
constructor( address _eventImplementation, address _rJoe, address _wavax, address _penaltyCollector, address _router, address _factory ) { // ... eventImplementation = _eventImplementation; // ... wavax = _wavax; // ... }
In RocketJoeFactory.sol, eventImplementation and wavax will never change, use immutable variable instead of storage variable can save gas.
#0 - cryptofish7
2022-01-31T00:27:15Z
Duplicate of #284
π Selected for report: WatchPug
Also found by: Czar102, Dravee, Jujic, Meta0xNull, byterocket, defsec, p4st13r4, pauliax, robee, sirhashalot
1.2609 USDT - $1.26
WatchPug
Every reason string takes at least 32 bytes.
Use short reason strings that fits in 32 bytes or it will become more expensive.
Instances include:
require( _startTime > block.timestamp, "RocketJoeStaking: rJOE minting needs to start after the current timestamp" );
require( user.amount >= _amount, "RocketJoeStaking: withdraw amount exceeds balance" );
require( rocketJoeFactory.isRJLaunchEvent(msg.sender), "RocketJoeToken: caller is not a RJLaunchEvent" );
require( address(rocketJoeFactory) == address(0), "RocketJoeToken: already initialized" );
require( _eventImplementation != address(0) && _rJoe != address(0) && _wavax != address(0) && _penaltyCollector != address(0) && _router != address(0) && _factory != address(0), "RJFactory: Addresses can't be null address" );
require( getRJLaunchEvent[_token] == address(0), "RJFactory: token has already been issued" ); require(_issuer != address(0), "RJFactory: issuer can't be 0 address"); require(_token != address(0), "RJFactory: token can't be 0 address"); require(_token != wavax, "RJFactory: token can't be wavax"); require( _tokenAmount > 0, "RJFactory: token amount needs to be greater than 0" ); require( IJoeFactory(factory).getPair(_token, wavax) == address(0) || IJoePair(IJoeFactory(factory).getPair(_token, wavax)) .totalSupply() == 0, "RJFactory: liquid pair already exists" );
require( _duration > PHASE_ONE_NO_FEE_DURATION, "RJFactory: phase one duration lower than no fee duration" );
require( _noFeeDuration < PHASE_ONE_DURATION, "RJFactory: no fee duration bigger than phase one duration" );
require( _maxWithdrawPenalty <= 5e17, "LaunchEvent: maxWithdrawPenalty too big" ); // 50% require( _fixedWithdrawPenalty <= 5e17, "LaunchEvent: fixedWithdrawPenalty too big" ); // 50% require( _userTimelock <= 7 days, "LaunchEvent: can't lock user LP for more than 7 days" ); require( _issuerTimelock > _userTimelock, "LaunchEvent: issuer can't withdraw before users" ); require( _auctionStart > block.timestamp, "LaunchEvent: start of phase 1 cannot be in the past" );
require(msg.sender != issuer, "LaunchEvent: issuer cannot participate"); require( msg.value > 0, "LaunchEvent: expected non-zero AVAX to deposit" );
require( user.balance > 0, "LaunchEvent: expected user to have non-zero balance to perform emergency withdraw" );
π Selected for report: WatchPug
Also found by: Dravee, Jujic, Rhynorater, TomFrenchBlockchain, defsec, hyh, ye0lde
2.3783 USDT - $2.38
WatchPug
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.
For example:
if (newAllocation > user.allocation) { // Burn tokens and update allocation. rJoeNeeded = getRJoeAmount(newAllocation - user.allocation); // ... }
newAllocation - user.allocation will never underflow.
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.amount - _amount will never underflow.
π Selected for report: WatchPug
Also found by: Ruhum, TomFrenchBlockchain, WatchPug, byterocket, hyh, kirk-baird
3.9149 USDT - $3.91
WatchPug
Every call to an external contract costs a decent amount of gas. For optimization of gas usage, external call results should be cached if they are being used for more than one time.
For example:
factory.getPair(wavaxAddress, tokenAddress) and factory.getPair(tokenAddress, wavaxAddress) in LaunchEvent#createPair()
function createPair() external isStopped(false) atPhase(Phase.PhaseThree) { // ... require( factory.getPair(wavaxAddress, tokenAddress) == address(0) || IJoePair( IJoeFactory(factory).getPair(wavaxAddress, tokenAddress) ).totalSupply() == 0, "LaunchEvent: liquid pair already exists" ); // ... pair = IJoePair(factory.getPair(tokenAddress, wavaxAddress)); // ... }
note: factory.getPair(a, b) δΈ factory.getPair(b, a) ηΈε, see
code at github
or
code at avascan
getPair[token0][token1] = pair; getPair[token1][token0] = pair; // populate mapping in the reverse direction
IJoeFactory(factory).getPair(_token, wavax) in RocketJoeFactory#createRJLaunchEvent()
require( IJoeFactory(factory).getPair(_token, wavax) == address(0) || IJoePair(IJoeFactory(factory).getPair(_token, wavax)) .totalSupply() == 0, "RJFactory: liquid pair already exists" );
token.decimals() in LaunchEvent#createPair()
if ( floorPrice > (wavaxReserve * 10**token.decimals()) / tokenAllocated ) { tokenAllocated = (wavaxReserve * 10**token.decimals()) / floorPrice; // ... }
#0 - cryptofish7
2022-01-31T22:55:29Z
π Selected for report: WatchPug
Also found by: Ruhum, TomFrenchBlockchain, WatchPug, byterocket, hyh, kirk-baird
WatchPug
function createPair() external isStopped(false) atPhase(Phase.PhaseThree) { (address wavaxAddress, address tokenAddress) = ( address(WAVAX), address(token) ); // ... if ( floorPrice > (wavaxReserve * 10**token.decimals()) / tokenAllocated ) { tokenAllocated = (wavaxReserve * 10**token.decimals()) / floorPrice; // ... } WAVAX.approve(address(router), wavaxReserve); token.approve(address(router), tokenAllocated); // ... }
WAVAX at L407 is already cached in the local variable wavaxAddress at L378 ~ L379.token at L396, L398, L408 is already cached in the local variable tokenAddress at L378 ~ L380.Reusing the cached local variable instead of reading from storage again can save gas.
Read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).
#0 - cryptofish7
2022-01-31T14:19:11Z
Duplicate of #236
π Selected for report: WatchPug
Also found by: Ruhum, TomFrenchBlockchain, robee
7.2498 USDT - $7.25
WatchPug
For the storage variables that will be accessed multiple times, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).
For example:
if (newAllocation > user.allocation) { // Burn tokens and update allocation. rJoeNeeded = getRJoeAmount(newAllocation - user.allocation); // ... }
user.allocation can be cached.
Change to:
uint256 oldAllocation = user.allocation; if (newAllocation > oldAllocation) { // Burn tokens and update allocation. rJoeNeeded = getRJoeAmount(newAllocation - oldAllocation); // ... }
if (tokenReserve > 0) { uint256 amount = tokenReserve; tokenReserve = 0; token.transfer(msg.sender, amount); }
tokenReserve can be cached.
Change to:
uint256 amount = tokenReserve; if (amount > 0) { tokenReserve = 0; token.transfer(msg.sender, amount); }
function currentPhase() public view returns (Phase) { if (block.timestamp < auctionStart || auctionStart == 0) { return Phase.NotStarted; } else if (block.timestamp < auctionStart + PHASE_ONE_DURATION) { return Phase.PhaseOne; } else if ( block.timestamp < auctionStart + PHASE_ONE_DURATION + PHASE_TWO_DURATION ) { return Phase.PhaseTwo; } return Phase.PhaseThree; }
auctionStart can be cached.
Change to:
function currentPhase() public view returns (Phase) { uint256 auctionStart_ = auctionStart; if (block.timestamp < auctionStart_ || auctionStart_ == 0) { return Phase.NotStarted; } else if (block.timestamp < auctionStart_ + PHASE_ONE_DURATION) { return Phase.PhaseOne; } else if ( block.timestamp < auctionStart_ + PHASE_ONE_DURATION + PHASE_TWO_DURATION ) { return Phase.PhaseTwo; } return Phase.PhaseThree; }
π Selected for report: WatchPug
39.7792 USDT - $39.78
WatchPug
function createRJLaunchEvent( address _issuer, uint256 _phaseOneStartTime, address _token, uint256 _tokenAmount, uint256 _tokenIncentivesPercent, uint256 _floorPrice, uint256 _maxWithdrawPenalty, uint256 _fixedWithdrawPenalty, uint256 _maxAllocation, uint256 _userTimelock, uint256 _issuerTimelock ) external override returns (address) { require( getRJLaunchEvent[_token] == address(0), "RJFactory: token has already been issued" ); require(_issuer != address(0), "RJFactory: issuer can't be 0 address"); require(_token != address(0), "RJFactory: token can't be 0 address"); require(_token != wavax, "RJFactory: token can't be wavax"); require( _tokenAmount > 0, "RJFactory: token amount needs to be greater than 0" ); require( IJoeFactory(factory).getPair(_token, wavax) == address(0) || IJoePair(IJoeFactory(factory).getPair(_token, wavax)) .totalSupply() == 0, "RJFactory: liquid pair already exists" ); // ... }
_issuer != address(0), _token != address(0), _tokenAmount > 0 are cheaper than other checks who read storage or do external call.
Therefore, checking _issuer != address(0), _token != address(0), _tokenAmount > 0 first can save some gas.
Change to:
function createRJLaunchEvent( address _issuer, uint256 _phaseOneStartTime, address _token, uint256 _tokenAmount, uint256 _tokenIncentivesPercent, uint256 _floorPrice, uint256 _maxWithdrawPenalty, uint256 _fixedWithdrawPenalty, uint256 _maxAllocation, uint256 _userTimelock, uint256 _issuerTimelock ) external override returns (address) { require(_issuer != address(0), "RJFactory: issuer can't be 0 address"); require(_token != address(0), "RJFactory: token can't be 0 address"); require( _tokenAmount != 0, "RJFactory: token amount needs to be greater than 0" ); require( getRJLaunchEvent[_token] == address(0), "RJFactory: token has already been issued" ); require(_token != wavax, "RJFactory: token can't be wavax"); require( IJoeFactory(factory).getPair(_token, wavax) == address(0) || IJoePair(IJoeFactory(factory).getPair(_token, wavax)) .totalSupply() == 0, "RJFactory: liquid pair already exists" ); // ... }
#0 - cryptofish7
2022-01-31T22:38:11Z
17.9007 USDT - $17.90
WatchPug
IJoeFactory private factory;
IJoeFactory(factory).getPair(wavaxAddress, tokenAddress)
factory is defined as IJoeFactory already, the type casting is redundant.
#0 - cryptofish7
2022-01-31T23:17:50Z
Should be 0.
#1 - dmvt
2022-02-22T19:33:08Z
I'm going to change this to a gas optimization since that's the only real benefit gained by making the change. I'm not sure that the change actually makes the code more clear, so non-critical doesn't really fit.
π Selected for report: WatchPug
Also found by: Dravee, TomFrenchBlockchain, wuwe1
7.2498 USDT - $7.25
WatchPug
contract LaunchEvent is Ownable {
The LaunchEvent.sol contract never utilized onlyOwner / owner() or any other features provided by the Ownable library.
Therefore, is Ownable can be removed.
#0 - cryptofish7
2022-01-31T23:01:39Z
Should be 0 severity.
Fix; https://github.com/traderjoe-xyz/rocket-joe/commit/989bb6ccf0b44b8f7f120a5f893b701118c1d9a6
#1 - dmvt
2022-02-22T19:47:02Z
Leaving the unnecessary contract out will save gas depending on how you deploy.
WatchPug
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); }
Since _amount can be 0. Checking if (_amount != 0) before the transfer can potentially save an external call and the unnecessary gas cost of a 0 token transfer.
When withdraw(0), user.amount = user.amount - _amount; can be skipped, to save storage write and external call of joe.safeTransfer(address(msg.sender), _amount);.
#0 - cryptofish7
2022-01-30T23:56:22Z
Duplicate of #317
#1 - dmvt
2022-02-23T12:06:38Z
This does point to a valid gas optimization. Rewards could be disbursed without sending a corresponding zero transfer. Using an if statement here would have a net positive impact on gas. Even better would be to split into the more common withdraw / withdrawAndClaim pattern.