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: 7/39
Findings: 4
Award: $2,063.19
🌟 Selected for report: 5
🚀 Solo Findings: 1
🌟 Selected for report: kirk-baird
1556.9237 USDT - $1,556.92
kirk-baird
The function allowEmergencyWithdraw() may be called by the rocketJoeFactory.owner() at any time. If it is called while the protocol is in Stage 3 and a pair has been created then the LP tokens will be locked and both issues and depositors will be unable to withdraw.
If allowEmergencyWithdraw() is called stopped is set to true. As a result functions withdrawIncentives() and withdrawLiquidity() will revert due to the isStopped(false) modifier reverting.
Additionally, emergencyWithdraw() will revert since all the WAVAX and token balances have been transferred to the liquidity pool.
Thus, depositors and issuers will have no methods of removing their LP tokens or incentives.
Consider adding the requirement require(address(pair) != address(0), "LaunchEvent: pair not created"); to the function allowEmergencyWithdraw().
#0 - cryptofish7
2022-02-01T00:56:09Z
To fix, we allow withdrawal of LP in emergencyWithdraw(): https://github.com/traderjoe-xyz/rocket-joe/commit/8a93c43e9972a2cf7c8ee04ccf263a405ecfcecc
🌟 Selected for report: kirk-baird
420.3694 USDT - $420.37
kirk-baird
Note: this attack requires rJoe to relinquish control during tranfer() which under the current RocketJoeToken it does not. Thus this vulnerability is raised as medium rather than high. Although it's not exploitable currently, it is a highly risky code pattern that should be avoided.
This vulnerability would allow the entire rJoe balance to be drained from the contract.
The function deposit() would be vulnerable to reentrancy if rJoe relinquished control flow.
The following lines show the reward calculations in variable pending. These calculations use two state variables user.amount and user.rewardDebt. Each of these are updated after _safeRJoeTransfer().
Thus if an attacker was able to get control flow during the rJoe::tranfer() function they would be able to reenter deposit() and the value calculated for pendingwould be the same as the previous iteration hence they would again be transferred pending rJoe tokens. During the rJoe transfer the would again gain control of the execution and call deposit() again. The process could be repeated until the entire rJoe balance of the contract has been transferred to the attacker.
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;
n/a
There are two possible mitigations. First is to use the openzeppelin reentrancy guard over the deposit() function which will prevent multiple deposits being made simultaneously.
The second mitigation is to follow the checks-effects-interactions pattern. This would involve updating all state variables before making any external calls.
#0 - cryptofish7
2022-01-31T20:06:58Z
Disagree with severity
#1 - dmvt
2022-02-22T13:18:43Z
I agree with the warden's assessment of risk on this one. Leaving it unaddressed would represent a potential future compromise if it was forgotten about by the team.
13.5716 USDT - $13.57
kirk-baird
It is possible to front-run the function setRJoe() by calling intiailize() on RocketJoeToken.
By calling RocketJoeToken.initialize() before setRJoe() the call to setRJoe() will fail. This is because RocketJoeToken.initialize() will fail if rocketJoeFactory is already set and it is set in initialize() as seen by the following snippet.
require( address(rocketJoeFactory) == address(0), "RocketJoeToken: already initialized" ); rocketJoeFactory = IRocketJoeFactory(msg.sender);
The impact is that the owner of RocketJoeFactory would be unable to call setRJoe() for as long as their transactions are being front-run.
n/a
n/a
Consider using a constructor for RocketJoeToken instead of the initialize() function and have rocketJoeFactory as a parameter to the constructor.
Then during RocketJoeFactory.setRJoe() simply ensure _rJoe.rocketJoeFactory() == address(this).
#0 - cryptofish7
2022-01-31T18:56:27Z
Duplicate of #8
#1 - cryptofish7
2022-01-31T22:56:59Z
🌟 Selected for report: kirk-baird
10.7404 USDT - $10.74
kirk-baird
The logic in _atPhase()` can be simplified to save gas and code complexity.
The code can be simplified to the follwoing.
function _atPhase(Phase _phase) internal view { require(currentPhase() == _phase, "LaunchEvent: incorrect phase"); }
n/a
n/a
Consider updating the code to that procided above.
#0 - cryptofish7
2022-02-01T01:20:19Z
🌟 Selected for report: kirk-baird
Also found by: Czar102
17.9007 USDT - $17.90
kirk-baird
Reduces gas useage for the contract.
SLOAD instructions cost about ~100 gas for each call on a slot (not including the first call). These values can be cached in memory to reduce the gas footprint.
The function deposit() makes four SLOADs on user.amount.
The function withdraw() makes four SLOADs on user.amount.
n/a
For each of these functions caching user.amount and performing all operations on the cached variable before writing it to storage after the last use would save ~300 gas.
🌟 Selected for report: WatchPug
Also found by: Ruhum, TomFrenchBlockchain, WatchPug, byterocket, hyh, kirk-baird
kirk-baird
The function createPair() makes duplicate external calls.
The external call factory.getPair(wavaxAddress, tokenAddress) is made twice in the function.
Note also the second external call IJoeFactory(factory).getPair(wavaxAddress, tokenAddress) factory is already of type IJoeFactory and so does not need to be cast.
require( factory.getPair(wavaxAddress, tokenAddress) == address(0) || IJoePair( IJoeFactory(factory).getPair(wavaxAddress, tokenAddress) ).totalSupply() == 0, "LaunchEvent: liquid pair already exists" );
Consider making one external call and caching the result in memory.
#0 - cryptofish7
2022-01-31T14:20:22Z
Duplicate of #236
🌟 Selected for report: kirk-baird
39.7792 USDT - $39.78
kirk-baird
Three external calls are made to rocketJoeFactory. These are seen in the following snipper from lines #261-263
PHASE_ONE_DURATION = rocketJoeFactory.PHASE_ONE_DURATION(); PHASE_ONE_NO_FEE_DURATION = rocketJoeFactory.PHASE_ONE_NO_FEE_DURATION(); PHASE_TWO_DURATION = rocketJoeFactory.PHASE_TWO_DURATION();
Since rocketJoeFactory is the msg.sender these variables may instead be passed as function parameters.
Consider updating LaunchEvent.initialize() and ILaunchEvent.initialize() to have three extra parameters.