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: 11/39
Findings: 4
Award: $1,557.28
🌟 Selected for report: 8
🚀 Solo Findings: 0
🌟 Selected for report: Ruhum
Also found by: TomFrenchBlockchain
TomFrenchBlockchain
Any address which holds any of the tokens to be issued can put LaunchEvent in a state where it needs to have an emergency shutdown. Issuers will retain all of their tokens minus incentives paid out and users will not receive anything in return for their RocketJoe (except if they're quick enough to claim the incentives).
Once the launch is at phase 3, it is possible to call the createPair function in order to create a pair on TraderJoe. This function will always fail if someone has added initial liquidity to the pool already.
This means that anyone who holds any of the token before the launch event concludes can prevent it from successfully completing by providing liquidity to this pool.
We expect that the only party which holds these tokens will be the issuer, however this gives them a free option to back out of the launch event by LPing this pool should they wish to do so (maybe due to an unfavourable token price). Also, should other parties hold any tokens they could also do this in order to grief.
At this point, nothing can be done unless the TraderJoe admin moves to shutdown the launch event. The issuer will receive all of their tokens (minus any incentives claimed before the event is put into shutdown) and users will receive their AVAX however they will have lost any RocketJoe they burned to participate (something which clearly has value as it enables participation in launches).
Comparing the spec for a Medium issue:
Assets 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.
This issue is then medium severity as availability is affected (requirement for emergency shutdown) and value is leaked (loss of RocketJoe, incentives paid out by issuer despite receiving no liquidity) under external requirements (malicious / uninformed holder of the token being launched)
Consider allowing RocketJoe to be transferred to the LaunchEvent contract to be burned in the case of a successful launch and refunded to users if it's unsuccessful.
Consider ways to handle existing liquidity in the pool. Sensible slippage limits would need to be set and ways to handle the situation where the pool is initialised to have a unrealistically high token price. (we don't need to handle the situation where the token is unrealistically undervalued as users will just buy it up to near the launch price)
#0 - cryptofish7
2022-01-31T14:25:23Z
Duplicate of #121
🌟 Selected for report: cccz
Also found by: 0x1f8b, Dravee, TomFrenchBlockchain, UncleGrandpa925, WatchPug, bobi, byterocket, hack3r-0m, sirhashalot
TomFrenchBlockchain
Tokens which don't implement the ERC20 standard properly can be locked on LaunchEvent under certain conditions.
In a number of places we perform a simple transfer of the token being launched
The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.
If there's a situation where the token transfer fails when a user is withdrawing it from LaunchEvent then the transaction will continue but the user will receive no tokens. The state change in LaunchEvent will prevent them from trying again so these tokens will be locked, leaking value.
Use the SafeERC20 library implementation from OpenZeppelin and call safeTransfer or safeTransferFrom when transferring ERC20 tokens.
#0 - cryptofish7
2022-01-31T14:47:14Z
Duplicate of #12
🌟 Selected for report: TomFrenchBlockchain
518.9746 USDT - $518.97
TomFrenchBlockchain
LaunchEvent pays out fewer incentives than expected.
When creating a launch event, issuers must provide the total amount of tokens they want to send to the contract and what percentage of these are reserved for incentives.
Note that there's an inconsistency between the documentation and the implementation. Documentation implies that the issuer provides _tokenAmount tokens and an additional _tokenAmount * _tokenIncentivesPercent / 1e18 as an incentive whereas in reality they only provide _tokenAmount
This can be seen here:
For us to pay out the correct percentage of _tokenAmount as incentives would expect that amount to be (_tokenAmount * _tokenIncentivesPercent) / 1e18 however as can be seen we pay out _tokenAmount - (_tokenAmount * 1e18) / (1e18 + _tokenIncentivesPercent)
This will consistently pay out a smaller percentage of the total amount of tokens than _tokenIncentivesPercent.
Switch to having the issuer provide explicit amounts _tokenIssuanceAmount and _tokenIncentivesAmount to avoid mistakes about how percentages are handled.
Add tests to ensure that the contract is initialised with the correct state.
#0 - cryptofish7
2022-01-31T22:32:56Z
Disagree with severity, this is more of an explanation issue. Calculations are correct however.
Fix: https://github.com/traderjoe-xyz/rocket-joe/commit/dbd19cc400abb5863edfc0443dd408ba5ae3e99a
#1 - dmvt
2022-02-23T12:35:01Z
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.
As this is a documentation vs reality issue, I think low risk is reasonable
🌟 Selected for report: WatchPug
Also found by: Dravee, Jujic, Rhynorater, TomFrenchBlockchain, defsec, hyh, ye0lde
TomFrenchBlockchain
gas costs
Use of safemath here is unnecessary due to above if statement
Wrap this line in an unchecked block
#0 - cryptofish7
2022-01-31T11:59:35Z
Duplicate of #233
🌟 Selected for report: WatchPug
Also found by: Ruhum, TomFrenchBlockchain, WatchPug, byterocket, hyh, kirk-baird
TomFrenchBlockchain
Detailed description of the impact of this finding.
Can cache the number of decimals here to save external call
As above.
#0 - cryptofish7
2022-01-31T14:29:29Z
Duplicate of #236
🌟 Selected for report: WatchPug
Also found by: Ruhum, TomFrenchBlockchain, robee
TomFrenchBlockchain
gas costs
Here we SLOAD auctionStart potentially 3 times and PHASE_ONE_DURATION potentially twice.
These could be stored in memory to save gas.
As above.
#0 - cryptofish7
2022-01-31T11:34:36Z
Duplicate of #234
🌟 Selected for report: TomFrenchBlockchain
Also found by: pauliax
17.9007 USDT - $17.90
TomFrenchBlockchain
Gas costs
LaunchEvent.sol stores the durations of the various phases
However in all the cases where these are used it's to regenerate the start times of these phases.
This maths can be performed once to calculate the start times of these phases the phase which you're in can then be calculated from comparisons to block.timestamp rather than adding up durations again.
This would also remove one SLOAD as we don't need to store auctionStart in order to know where to calculate the beginnings of each phase from.
As above
🌟 Selected for report: TomFrenchBlockchain
39.7792 USDT - $39.78
TomFrenchBlockchain
Gas costs
LaunchEvent has many timestamps and durations in storage:
Realistically none of these require a full uint256, so we can restrict them to uint64 (or even smaller) to pack them into a single storage slot. This will remove a number of SLOADs to save gas.
Use smaller types where possible to pack storage variables together.
🌟 Selected for report: TomFrenchBlockchain
39.7792 USDT - $39.78
TomFrenchBlockchain
Detailed description of the impact of this finding.
Both these values are restricted to be less than 5e17 and so don't need a full uint256. We can then use a smaller type so that they fit in the same storage slot.
Use smaller types so that these values are packed into the same storage slot as other variables which are used in the same operation.
#0 - cryptofish7
2022-01-31T23:16:48Z
Impacts readability for little gain
🌟 Selected for report: TomFrenchBlockchain
39.7792 USDT - $39.78
TomFrenchBlockchain
Gas savings
Solidity 0.8.10 has a useful change which reduced gas costs of external calls which expect a return value: https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/
Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist
TraderJoe is using 0.8.6: https://github.com/code-423n4/2022-01-trader-joe/blob/a1579f6453bc4bf9fb0db9c627beaa41135438ed/hardhat.config.js#L12
Updating to the newer version of solc will allow TraderJoe to take advantage of these lower costs for external calls.
Update to 0.8.10+
🌟 Selected for report: TomFrenchBlockchain
39.7792 USDT - $39.78
TomFrenchBlockchain
Reduced gas costs of interacting with clones
The codebase makes use of clones for LaunchEvent.sol which results in storing many variables in storage which could otherwise be made immutable.
It is possible to use immutable variables with clones in order to avoid the associated cost of SLOADs with some calldata trickery. See this repo for an example of this: https://github.com/ZeframLou/clones-with-immutable-args
Switch to using clones which support immutable variables.
🌟 Selected for report: TomFrenchBlockchain
39.7792 USDT - $39.78
TomFrenchBlockchain
gas costs
LaunchEvent has an explicit initialized variable which is in storage.
https://github.com/code-423n4/2022-01-trader-joe/blob/a1579f6453bc4bf9fb0db9c627beaa41135438ed/contracts/LaunchEvent.sol#L228
To save an SSTORE we could just check that _auctionStart > 0 as this is a sufficient check for initialisation. If a getter is needed then a function like the below could be added.
function initialized() external view returns bool { return auctionStart > 0; }
As above.
#0 - cryptofish7
2022-01-31T23:35:11Z
🌟 Selected for report: TomFrenchBlockchain
39.7792 USDT - $39.78
TomFrenchBlockchain
Extra gas costs from storage reads/writes
The UserData struct uses full uint256s for balance and allocation.
It's very unlikely that someone will have enough avax or rJOE in order to get close to filling a uint256. We could then store these in smaller types such that these variables fit into the same storage slot.
If we used a uint120 for both these variables the entire UserData struct would fit in a single slot. As a TraderJoe pair only accepts balances that fit in a uint112 then we can safely make balance a uint120 without any extra assumptions. We then just need to make a (very loose) restriction on the maximum allocation value.
Enforce that maxAllocation fits in a uint120 and pack the UserData struct.
🌟 Selected for report: WatchPug
Also found by: Dravee, TomFrenchBlockchain, wuwe1
TomFrenchBlockchain
Extra deployment costs
LaunchEvent inherits from Ownable but doesn't make use of any of its functions.
We can then just remove this entirely to avoid having to deploy the bytecode
Remove inheritance.
#0 - cryptofish7
2022-01-31T11:16:37Z
Duplicate of #241