Platform: Code4rena
Start Date: 16/09/2021
Pot Size: $200,000 SUSHI
Total HM: 26
Participants: 16
Period: 14 days
Judge: alcueca
Total Solo HM: 13
Id: 29
League: ETH
Rank: 6/16
Findings: 3
Award: $11,966.10
🌟 Selected for report: 16
🚀 Solo Findings: 0
0xRajeev
If the user deposits less ETH than claimed by the params.tokenIn for native functions, then wETH is transferred from the msg.sender to pool, while the user's ETH is left behind in the Router.
Manual Analysis
For a safe user experience, Router should refund remaining ETH at the end of transaction.
#0 - alcueca
2021-10-27T04:52:51Z
Can't find a duplicate, @maxsam4?
#1 - maxsam4
2021-11-08T11:31:34Z
🌟 Selected for report: nikitastupin
0xRajeev
The DOMAIN_SEPARATOR which includes the chainID captured at contract construction time is used for permit signature verification. As specified in the security considerations of EIP-2612, this has the following concern: "If the DOMAIN_SEPARATOR contains the chainId and is defined at contract deployment instead of reconstructed for every signature, there is a risk of possible replay attacks between chains in the event of a fututre chain split." See https://eips.ethereum.org/EIPS/eip-2612.
Note: It looks like this has been fixed in the code after the contest started.
Before contest:
https://eips.ethereum.org/EIPS/eip-2612#security-considerations
Fixed after contest start:
Manual Analysis
It looks like this has been fixed appropriately in the code after the contest started.
#0 - maxsam4
2021-11-08T11:33:41Z
🌟 Selected for report: 0xRajeev
70.5984 SUSHI - $882.48
0xRajeev
The token address validation check of require(_token0 != address(this), “INVALID_TOKEN"); performed in the constructor of ConstantProductPool is missing in Hybrid and Index pools. It is not clear that these two pools do not require this validation check for any specific aspect of the pool type.
https://github.com/sushiswap/trident/blob/7b207a34c12dacc9e0eab603d77bc09f9b534b8e/contracts/pool/ConstantProductPool.sol#L60-L61 ] https://github.com/sushiswap/trident/blob/7b207a34c12dacc9e0eab603d77bc09f9b534b8e/contracts/pool/HybridPool.sol#L67-L68
Manual Analysis
Add the invalid token (where token address == Pool address) checks in constructors of Hybrid and Index pools.
🌟 Selected for report: 0xRajeev
70.5984 SUSHI - $882.48
0xRajeev
FlashSwap calculates the fee directly using fee = (amountIn * swapFee) / MAX_FEE; without using _handleFee() that accounts for the barFee multiple, unlike other functions that use _handleFee(). It is not specified/documented if barFee applies to flashSwap or if this is an incorrect fee calculation.
The fee handling in Hybrid Pool has been significantly optimized right after the contest started where it appears that neither flashSwap nor Swap now applies the barFee factor in fee calculation but it’s only used in _mintFee used by mint/burn functions. If the current optimized commit is the correct version, then the swap() code from the contest commit timeframe that applied barFee is incorrect.
Contest commit:
Current/Latest commit:
Manual Analysis
Evaluate if barFee is used correctly in fee calculations of flashSwap/swap functions.
🌟 Selected for report: 0xRajeev
0xRajeev
The _updateReserves() function in HybridPool uses a strict bound of ‘<‘ against type(uint128).max while updating the reserves. This is off-by-one strict bound because this technically can be '<=' without overflowing as done in ConstantProduct Pool.
Manual Analysis
Change ‘<‘ to ‘<=‘
#0 - alcueca
2021-10-27T04:51:32Z
Can't find a duplicate, @maxsam4?
9.2639 SUSHI - $115.80
0xRajeev
Contracts should be deployed using the same compiler version/flags with which they have been tested.
Using an unlocked pragma of >=0.8.0 which allows different latest versions of Solidity, (including potentially future breaking versions >=0.9.0) is risky because it allows contracts to be accidentally deployed using untested older/newer compiler versions with unfixed/undiscovered bugs.
https://twitter.com/solidity_lang/status/1442550177202679809
https://twitter.com/solidity_lang/status/1442902224263282688
Manual Analysis
Locking the pragma to a specific version (by not specifying a range, which is possible here because these contracts are not libraries meant to be used with different contracts across compiler versions) ensures that contracts get deployed with the same version as with that tested. Use one of the recent versions, e.g. 0.8.4 or 0.8.5, which has fixed compiler bugs from previous releases and has been around for a few months/releases to give time for any undiscovered/untested bugs to surface. For example, the latest version 0.8.8 was released and a bug was reported 1 day after release.
#0 - maxsam4
2021-10-21T06:49:28Z
Solidity version controlled via hardhat config.
#1 - alcueca
2021-10-24T06:02:25Z
Hardhat config is independent from contract code. Development and deployment could happen from different repositories (using submodule or npm), and therefore testing and deployment bytecode might differ.
🌟 Selected for report: 0xRajeev
70.5984 SUSHI - $882.48
0xRajeev
Zero-address checks for input validation of address-type variables is a best-practice. While this is implemented in most places, there are 1-2 missing ones.
Missing zero-address check here unless the intention is also to capture a renounce/burn functionality in which case a separate renounceMigrator() would be better to separate the concerns: https://github.com/sushiswap/trident/blob/6bd4c053b6213ffc612987eb565aa3813d5f0d13/contracts/deployer/MasterDeployer.sol#L66
Manual Analysis
Add zero-address check.
#0 - maxsam4
2021-10-21T06:48:18Z
Acceptable risk
🌟 Selected for report: 0xRajeev
70.5984 SUSHI - $882.48
0xRajeev
Setter functions for critical contract parameters accessible only by privileged roles e.g. admin should consider adding timelocks (along with emitted events) so that users and other privileged roles can detect upcoming changes and have the time to react to them.
Changes to whitelists, barFee and migrator address may have a financial or trust impact on users who should be given an opportunity to react to them by exiting/engaging without being surprised when such changes are made effective immediately.
See similar Medium-severity finding in ConsenSys's Audit of 1inch Liquidity Protocol (https://consensys.net/diligence/audits/2020/12/1inch-liquidity-protocol/#unpredictable-behavior-for-users-due-to-admin-front-running-or-general-bad-timing)
Manual Analysis
Consider adding timelocks to such contracts with critical setter functions.
#0 - maxsam4
2021-10-21T06:47:27Z
Acceptable risk. There is no critical function controlled by the owner.
🌟 Selected for report: 0xRajeev
70.5984 SUSHI - $882.48
0xRajeev
The addToWhitelist and removeFromWhitelist unconditionally set the whitelist values of whitelistedFactories for the requested factory without checking if _factory was already whitelisted or not. Checking before setting will help surface inconsistent views of contract state vs offchain/owner assumptions.
The same is true for setMigrator function where it will help to check that the address being set is different from the one already set.
Manual Analysis
Add a require to check that the whitelist is false/true before setting it to true/false. Add a require to check that _migrator != migrator.
#0 - maxsam4
2021-10-21T06:47:06Z
Acceptable risk. I don't see how this will cause irregularities in offchain workers.
🌟 Selected for report: 0xRajeev
70.5984 SUSHI - $882.48
0xRajeev
TridentOwnable splits the ownership transfer into a 2-step transfer+claim process which is desirable over the single-step transferOwnership usually seen. This can be made less riskier by adding a timelock to force a time delay between transfer and claim so that a malicious/compromised owner does not transfer ownership in two successive/immediate transactions.
Manual Analysis
Consider adding a timelock to force a time delay between transferOwner and claimOwner functions so that their events can be monitored and reacted to if necessary.
#0 - maxsam4
2021-10-21T06:46:27Z
Acceptable risk
🌟 Selected for report: 0xRajeev
70.5984 SUSHI - $882.48
0xRajeev
TridentOwnable splits the ownership transfer into a 2-step transfer+claim process which is desirable over the single-step transferOwnership usually seen. It however provides the single-step “direct” transfer as an option which is risky because it might be used by default, avoiding the less-riskier 2-step pending+claim indirect transfer.
Manual Analysis
Consider removing direct ownership transfer altogether to force a 2-step change or at least consider checking for (pendingOwner != 0) in the direct path so that it does not override once the indirect pendingOwner is set.
#0 - maxsam4
2021-10-21T06:46:10Z
Acceptable risk
🌟 Selected for report: 0xRajeev
70.5984 SUSHI - $882.48
0xRajeev
The _safeTransfer and _safeTransferFrom function calls make a low-level call() on the token address without checking if that is indeed a contract or not (EOA). EVM low-level calls return success even if the address used does not have a contract. So checking for success return value assumes the transfer happened successfully which it may have failed silently due to missing contract at token address for some reason (incorrect token address or selfdestruct).
These functions appear to be adapted from Uniswap’s TransferHelper which also does not have contract existence checks. However, similar OpenZeppelin functions perform contract existence checks for the reasons mentioned above.
https://github.com/Uniswap/uniswap-lib/blob/master/contracts/libraries/TransferHelper.sol
OpenZeppelin’s implementation and comment highlighting this issue: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6241995ad323952e38f8d405103ed994a2dcde8e/contracts/token/ERC20/utils/SafeERC20.sol#L87-L92
Manual Analysis
Add contract existence checks before making the low-level calls.
#0 - maxsam4
2021-10-21T06:45:06Z
this is intentional gas optimization
#1 - alcueca
2021-10-24T06:08:05Z
As a previously undisclosed vulnerability, I accept the severity rating.
🌟 Selected for report: 0xRajeev
32.2581 SUSHI - $403.23
0xRajeev
The calculations of balance are never used in later logic and therefore can be removed to save gas from the computations.
Manual Analysis
Remove unused code. It looks like this has been removed in the latest commits.
🌟 Selected for report: 0xRajeev
32.2581 SUSHI - $403.23
0xRajeev
Each staticcall (or any CALL* family function) costs 2600 gas along with additional gas costs for decoding the returned values. Consider a masterDeployer function that returns all three barFee, barFeeTo and bento to save >5200 gas for every pool deployment. This applies to other two pools as well.
Manual Analysis
The commits after contest start have replaced static calls with direct function calls, which saves gas cost from abi.decodes, but still have the same gas cost for the function call. Consider a masterDeployer function that returns all three barFee, barFeeTo and bento.
14.5161 SUSHI - $181.45
0xRajeev
In _computeLiquidityFromAdjustedBalances(), when s == 0, we can return after setting compute = 0 instead of falling through to code after that which does not change the return value of compute because D will also be 0.
Manual Analysis
Add an explicit return statement after L345
#0 - alcueca
2021-10-27T04:53:18Z
Can't find a duplicate, @maxsam4?
#1 - maxsam4
2021-11-08T11:33:10Z
🌟 Selected for report: 0xRajeev
32.2581 SUSHI - $403.23
0xRajeev
The codebase already uses the unchecked{} primitive to save gas where computation is known to be overflow/underflow safe. There are a few more places where this can be applied.
balance is guaranteed to be >= reserve across control flows and so the following can be made unchecked:
Manual Analysis
It looks like this was optimized just after the contest began.
🌟 Selected for report: 0xRajeev
32.2581 SUSHI - $403.23
0xRajeev
In the IndexPool constructor, while updating the state variable totalWeight, caching intermediate totals in a local variable and then writing only once to the state variable outside the loop will save on multiple SSTOREs and SLOADs.
Caching cachedMsgSender state variable outside the loop in a local variable and using that in calls within the loop will avoid SLOADs given that this state variable value is the same for all loop iterations.
Hoisting pools[token0][token1] out of the loop to cache it in a local/memory array will prevent repetitive SLOADs in every loop iteration.
Manual Analysis
Cache intermediate total weights in a local variable and then write only once to the state variable totalWeight outside the loop.
Cache cachedMsgSender state variable outside the loop in a local variable and use that in calls within the loop.
Hoist pools[token0][token1] out of the loop to cache it in a local/memory array
In general, when the same state variable is read multiple times within a function, cache it in a local variable at the function beginning and use that to avoid SLOADs which cost 2100/100 gas (after Berlin upgrade) depending on cold/warm and are replaced by MLOADs which only cost 3 gas units.
🌟 Selected for report: 0xRajeev
32.2581 SUSHI - $403.23
0xRajeev
The local variable used as for loop index need not be initialized to 0 because the default value is 0. Avoiding this anti-pattern can save a few opcodes and therefore a tiny bit of gas.
and some other places.
Manual Analysis
Remove explicit 0 initialization of for loop index variable.
🌟 Selected for report: 0xRajeev
32.2581 SUSHI - $403.23
0xRajeev
The ecrecover function is used to verify and execute EIP-2612 permit transactions. The built-in EVM precompile ecrecover is susceptible to signature malleability (because of non-unique s and v values) which could lead to replay attacks (references: https://swcregistry.io/docs/SWC-117, https://swcregistry.io/docs/SWC-121 and https://medium.com/cryptronics/signature-replay-vulnerabilities-in-smart-contracts-3b6f7596df57).
While this is not exploitable for replay attacks in the current implementation because of the use of nonces, this may become a vulnerability if used elsewhere.
Manual Analysis
Consider using OpenZeppelin’s ECDSA library (which prevents this malleability) instead of the built-in function: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/cryptography/ECDSA.sol
#0 - alcueca
2021-10-27T04:52:24Z
Can't find a duplicate, @maxsam4?
#1 - maxsam4
2021-11-08T11:35:25Z
Can't find the duplicate but the issue is not relevant anyway. We mitigated the issue by using a nonce, as mentioned in the issue itself.
#2 - alcueca
2021-11-09T05:14:17Z
Wouldn't be cheaper to use OpenZeppelin's version? Updating the nonce is an SSTORE.