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: 5/39
Findings: 5
Award: $2,357.40
🌟 Selected for report: 3
🚀 Solo Findings: 1
🌟 Selected for report: harleythedog
Also found by: sirhashalot
sirhashalot
The RocketJoeStaking.sol _safeRJoeTransfer() function doesn't use the OpenZeppelin SafeERC20 safeTransfer() function. Instead, it uses the standard transfer() function and applies logic to avoid withdrawing more rJoe than is available. Using a function name with the word "safe" when the function does not check the return values of function calls is misleading and can lead to security issues with later misuse. Because safeTransfer() is used elsewhere in this contract for the joe token, so most likely this is an oversight and safeTransfer() for the rJoe token was intended.
The _safeRJoeTransfer function in RocketJoeStaking.sol does not check the function return value and therefore does not deserve the name safe. Compare the _safeRJoeTransfer() function to the _safeTransferAVAX() function in LaunchEvent.sol, which does check the return value.
If a function's name includes with word "safe", it should check the function return value. Use safeTransfer() in the _safeRJoeTransfer() function. If safeTransfer is not needed, rename the function to rJoeTransferIfAvailable or similar.
#0 - cryptofish7
2022-01-31T14:51:58Z
Duplicate of #170
🌟 Selected for report: sirhashalot
1556.9237 USDT - $1,556.92
sirhashalot
The LaunchEvent.sol createPair() function calls router.addLiquidity() with a amountADesired == amountAMin and amountBDesired == amountBMin. Because there is no allowance for slippage, if the zero slippage requirement is not met then the addLiquidity() function will revert and prevent users from using the createPair() function. This could be caused either by frontrunning the createPair call or in a situation where the liquidity pool exists but does not allow for zero slippage with the assets it is holding.
The zero slippage addLiquidity call is found in LaunchEvent.sol. This code may have been written with the assumption that only Rocket Joe will have a balance of the new token, so no other user could call the addLiquidity function with both assets, since the whitepaper states "Rocket Joe liquidity launch will complete before launchpad public sale release any tokens to the public". However, the new token contract should be considered untrusted and Rocket Joe cannot guarantee where all the new tokens are before phase 3 of the Rocket Joe launch event, which is when createPair() is called. The token creator who has control over the token allocation is not controlled by Trader Joe, so an attacker who has early access to the new token can break the outlined assumptions.
Consider how the launch event functions may break if the new token is launched by an attacker who doesn't follow the assumptions outlined. One solution for this createPair() issue is to add an input parameter to the function to handle a slippage allowance.
🌟 Selected for report: cccz
Also found by: 0x1f8b, Dravee, TomFrenchBlockchain, UncleGrandpa925, WatchPug, bobi, byterocket, hack3r-0m, sirhashalot
sirhashalot
The LaunchEvent.sol contract handles arbitrary ERC20 tokens for launch events. These ERC20 tokens may have unexpected behavior such as unexpected return values (like these 130 tokens). By using the standard ERC20 functions and not checking the return values of these functions, a malicious token could be created to exploit the launch event code. Observe that safeTransfer() is used for the JOE token in RocketJoeStaking.sol but this is not consistent across all contracts.
This issue is most prominent in LaunchEvent.sol, with several ERC20 transfer() calls like this one on line 458.
Use OpenZeppelin safeTransfer() functions to improve edge case handling by checking the return value of the transfer() function.
#0 - cryptofish7
2022-01-31T14:46:52Z
Duplicate of #12
#1 - dmvt
2022-02-22T10:51:25Z
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: sirhashalot
Also found by: 0v3rf10w, 0x1f8b, Dravee, UncleGrandpa925, cccz, defsec, gzeon
sirhashalot
Some functions in RocketJoeFactory.sol have zero checks for setting specific state variables, but there zero address checks are not always applied. Setting some of these state variables to the zero address, whether intentional or not, can break the protocol functionality. Adding these checks consistently would prevent this scenario.
The constructor in RocketJoeFactory.sol performs zero address checks before setting the router, factory, penaltyCollector, and rJoe state variables. Later in the same contract, the functions setRJoe(), setPenaltyCollector(), setRouter(), and setFactory() omit the same zero address checks that were applied earlier. Since the issues that can be caused by setting these state variables to the zero address exist whether setting the value in the constructor or in the setter function, these checks should be applied consistently.
Add zero address checks in the setter functions for these state variables just like is done in the constructor. If it is determined that a zero check for any of these state variables is not needed, then the zero check can be removed from the RocketJoeFactory.sol constructor for consistency.
#0 - cryptofish7
2022-01-31T22:23:39Z
🌟 Selected for report: WatchPug
Also found by: Czar102, Dravee, Jujic, Meta0xNull, byterocket, defsec, p4st13r4, pauliax, robee, sirhashalot
sirhashalot
Strings are broken into 32 byte chunks for operations. Revert error strings over 32 bytes therefore consume extra gas as documented publicly
There are multiple examples of this gas optimization opportunity, including but not limited to:
Reducing revert error strings to under 32 bytes decreases deployment time gas and runtime gas when the revert condition is met. Alternatively, the code could be modified to use custom errors, introduced in Solidity 0.8.4: https://blog.soliditylang.org/2021/04/21/custom-errors/
#0 - cryptofish7
2022-02-11T01:08:19Z
Duplicate of #242
🌟 Selected for report: sirhashalot
Also found by: Dravee, Rhynorater, robee
7.2498 USDT - $7.25
sirhashalot
The withdrawAVAX() function of LaunchEvent.sol and initialize() function of RocketJoeStaking.sol can be declared external for gas savings
Declare functions as external instead of public when possible
#0 - cryptofish7
2022-01-31T19:38:53Z