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: 17/39
Findings: 2
Award: $952.01
🌟 Selected for report: 5
🚀 Solo Findings: 0
🌟 Selected for report: cccz
Also found by: 0x1f8b, Dravee, TomFrenchBlockchain, UncleGrandpa925, WatchPug, bobi, byterocket, hack3r-0m, sirhashalot
60.3184 USDT - $60.32
cccz
It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L457
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L463
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L489
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L513
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L537
Manual analysis
Consider using safeTransfer/safeTransferFrom or require() consistently.
#0 - cryptofish7
2022-02-01T00:58:15Z
#1 - dmvt
2022-02-21T12:25:09Z
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.
#2 - amarpatel
2023-03-17T19:56:13Z
To test, I did this:
on package.json add test:TEMP to scripts:
... "test:TEMP": "NODE_ENV=test ETHERNAL_ENABLED=false hardhat test --grep \"should process subsequent withdrawals of all supported tokens successfully\" --parallel", ...
Every vault fails: <img width="134" alt="image" src="https://user-images.githubusercontent.com/5858247/226017325-b8bd2185-071f-485a-9a6b-f4ab5e81ac8d.png">
233.5386 USDT - $233.54
cccz
The current ownership transfer process involves the current owner calling transferOwnership(). This function checks the new owner is not the zero address and proceeds to write the new owner’s address into the owner’s state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier.
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeToken.sol#L6
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L5
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeStaking.sol#L7
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L6
Manual analysis
Implement zero address check and consider implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.
#0 - dmvt
2022-02-21T12:29:47Z
I agree that the severity for this is too high. While the functionality of the protocol could be effected, the likelihood of this occurring is very low.
🌟 Selected for report: sirhashalot
Also found by: 0v3rf10w, 0x1f8b, Dravee, UncleGrandpa925, cccz, defsec, gzeon
cccz
Missing 0 address check when setting address in RocketJoeFactory contract
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L158-L162
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L166-L173
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L177-L180
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L184-L187
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L191-L194
Manual analysis
Add 0 address check
#0 - cryptofish7
2022-01-31T00:52:55Z
Duplicate of #263
13.5716 USDT - $13.57
cccz
In LaunchEvent/RocketJoeStaking/RocketJoeToken contract, the initialize function was missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L215-L286
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeStaking.sol#L57-L76
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeToken.sol#L25-L32
Manual analysis
Setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers
🌟 Selected for report: cccz
518.9746 USDT - $518.97
cccz
The RocketJoeFactory contract assumes that _wavax is WAVAX, but does not check it in the constructor. If the input address does not meet the assumptions, which could lead to wrong market liquidity and prices calculations.
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/RocketJoeFactory.sol#L66
Manual analysis
Check if the input address matches the assumption
🌟 Selected for report: cccz
Also found by: csanuragjain, defsec, robee
94.5831 USDT - $94.58
cccz
Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value.They must first be approved by zero and then the actual allowance must be approved.
https://github.com/code-423n4/2022-01-trader-joe/blob/main/contracts/LaunchEvent.sol#L407
Manual analysis
WAVAX.approve(address(router), wavaxReserve); + token.approve(address(router), 0); token.approve(address(router), tokenAllocated);
#0 - cryptofish7
2022-02-01T00:47:09Z
Removed router.addLiquidity() and replaced with pair.mint(), so approve not necessary anymore