Platform: Code4rena
Start Date: 20/10/2021
Pot Size: $30,000 ETH
Total HM: 5
Participants: 15
Period: 3 days
Judge: 0xean
Total Solo HM: 3
Id: 44
League: ETH
Rank: 3/15
Findings: 3
Award: $4,127.54
🌟 Selected for report: 11
🚀 Solo Findings: 1
🌟 Selected for report: pants
2448.4207 USDC - $2,448.42
pants
Users can call Swap.swapByQuote() to execute an ETH swap (where they receive ETH) without paying swap fee for the gained ETH. They can trick the system by setting zrxBuyTokenAddress to an address of a malicious contract and making it think they have executed an ERC20 swap when they have actually executed an ETH swap. In this case, the system will give them the ETH they got from the swap (boughtETHAmount) without charging any swap fees for it, because the systems consideres this ETH as "refunded ETH" that wasn't part of the "ERC20" swap.
Users can execute ETH swap without paying swap fees for the ETH the got from the swap.
The steps of the attack are:
M), that will be used for zrxBuyTokenAddress.Swap.swapByQuote() with zrxBuyTokenAddress=M and minimumAmountReceived=0. The rest of the arguments should specify our ETH swap, nothing special here.M to return 0 and 1 at the first and second times when fillZrxQuote calls zrxBuyTokenAddress.balanceOf(address(this)), respectively.boughtERC20Amount now equals 1 and the function will "return any refunded ETH" to the caller, without charging any swap fees on it. This ETH is actually the output of that ETH swap.Manual code review.
Charge swap fees for the "refunded ETH" on ERC20 swaps (when boughtERC20Amount > 0), or require boughtETHAmount == 0.
#0 - Shadowfiend
2021-11-04T17:10:27Z
Still working through whether this is an issue we're truly worried about; in particular, if you want to do this, you probably might as well use the 0x API to swap directly.
Nonetheless, it's overshadowed by #37, which will likely lead to changes that will make this impractical as well.
#1 - 0xean
2021-11-06T13:22:55Z
Downgrading to severity 2 as this would lead to "leaked value" as only the fees are lost by the protocol in this attack vector and customer assets aren't being stolen.
pants
When Swap's constructor sets the initial swapFee to the value of the argument swapFee, it doesn't check that this value is smaller than SWAP_FEE_DIVISOR.
If swapFee equals SWAP_FEE_DIVISOR, then the system charges 100% fees so users have no reason to use it. If swapFee is greater than SWAP_FEE_DIVISOR, then the swaps won't work at all. Instead, they will always revert due to an underflow on SWAP_FEE_DIVISOR.sub(swapFee).
Manual code review.
Require swapFee_ < SWAP_FEE_DIVISOR in Swap's constructor.
#0 - Shadowfiend
2021-11-03T21:05:04Z
Duplicate of #25.
🌟 Selected for report: pants
816.1402 USDC - $816.14
pants
The file Swap.sol has an open TODO.
Open TODOs can hint at programming or architectural errors that still need to be fixed.
Manual code review.
Resolve this TODO and bubble up the error.
🌟 Selected for report: pants
79.974 USDC - $79.97
pants
This line in Swap.setSwapFee() perfoms an SLOAD operation for a value that is already stored in a local variable:
emit NewSwapFee(swapFee);
Storage reads are much more expensive than reading local variables.
Manual code review.
Use the already existing local variable instead of loading this value from storage:
emit NewSwapFee(swapFee_);
10.4942 USDC - $10.49
pants
The contract Swap uses OpenZeppelin's SafeMath library to avoid overflows and underflows in math calculations, although Solidity 0.8.0 and higher versions has built-in checks for overflows and underflows in math operations.
The use of SafeMath adds overhead and increase gas usage.
Manual code review.
Don't use this library in Swap.
#0 - Shadowfiend
2021-10-29T20:55:15Z
Duplicate of #42.
35.9883 USDC - $35.99
pants
The function Swap.sweepFees() loads the same array element (tokens[i]) more than once. In such cases, only one array boundaries check should take place, and the rest are unnecessary. Therefore, this array element should be cached in a local variable and then be loaded again using this local variable, skipping the redundent second array boundaries check.
Unnecessary array boundaries checks are taken place and increase gas costs.
Manual code review.
Load the array elements once, cache them in local variables and then read them again using the local variables. For example:
uint256 item = array[i]; // do something with `item` // do some other thing with `item`
🌟 Selected for report: harleythedog
Also found by: pants
35.9883 USDC - $35.99
pants
These lines in Swap.sweepFees() perfom SLOADs operations for the same feeRecipient value:
require( feeRecipient != address(0), "Swap::withdrawAccruedFees: feeRecipient is not initialized" ); IERC20(tokens[i]).safeTransfer(feeRecipient, balance); emit FeesSwept(tokens[i], balance, feeRecipient); feeRecipient.transfer(address(this).balance); emit FeesSwept(address(0), address(this).balance, feeRecipient);
Storage reads are much more expensive than reading local variables.
Manual code review.
Load feeRecipient from storage once, cache it in a local variable and them use this local variable instead of loading this value from storage again.
#0 - Shadowfiend
2021-11-03T21:44:05Z
Duplicate of #18.
pants
The for loop in Swap.sweepFees() follows this for-each pattern:
for (uint256 i = 0; i < array.length; i++) { // do something with `array[i]` }
In such for loops, the array.length is read on every iteration, instead of caching it once in a local variable and read it again using the local variable.
Calldata reads are a bit more expensive than reading local variables.
Manual code review.
Read these values from calldata once, cache them in local variables and then read them again using the local variables. For example:
uint256 length = array.length; for (uint256 i = 0; i < length; i++) { // do something with `array[i]` }
#0 - Shadowfiend
2021-11-04T17:00:59Z
Duplicate of #18.
#1 - 0xean
2021-11-06T11:57:42Z
This is similar but a different recommendation to #18 as this is referring to the repeated calls to .length which 18 doesn't suggest. Leaving as unique and valid
🌟 Selected for report: pants
79.974 USDC - $79.97
pants
There is no risk of overflow caused by increamenting the iteration index in for loops (the i++ in for (uint256 i = 0; i < numIterations; i++)).
Increaments perform overflow checks that are not necessary in this case.
Manual code review.
Surround the increament expressions with an unchecked { ... } block to avoid the default overflow checks. For example, change the loop
for (uint256 i = 0; i < numIterations; i++) { // ... }
to
for (uint256 i = 0; i < numIterations;) { // ... unchecked { i++; } }
It is a little less readable but it saves a significant amount of gas.
#0 - Shadowfiend
2021-10-29T20:58:07Z
The only for loop in Swap is in fee sweeping (https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#L250-L256). Since the tokens list is passed externally and the iteration variable is a uint8 with max value 255, I believe there is in fact risk of overflow (see also #80 ).
#1 - mhluongo
2021-11-05T00:14:30Z
We could check the bounds once before the loop rather than checking it each iteration though.
#2 - Shadowfiend
2021-11-05T00:49:56Z
Yeah there are a few other issues that cover that kind of thing under the auspices of gas optimization.
#3 - 0xean
2021-11-06T11:59:48Z
This is a valid optimization especially since the uint8 should be a uint256 based on other wardens comments. Leaving as valid and open
🌟 Selected for report: pants
79.974 USDC - $79.97
pants
The function Swap.sweepFees uses postfix increaments (x++) instead of prefix increaments (++x).
Prefix increaments are cheaper than postfix increaments.
Manual code review.
Change all postfix increaments to prefix increaments where it doesn't affects the functionality.
🌟 Selected for report: pants
79.974 USDC - $79.97
pants
These internal functions are never called in any contract that inherits their contract:
Swap.fillZrxQuote()Swap.signifiesETHOrZero()EmergencyGovernable.governor()Therefore, their visibility can be reduced to private.
private functions are cheaper than internal functions.
Manual code review.
Define these functions as private.
🌟 Selected for report: pants
79.974 USDC - $79.97
pants
This require statement is unnecessary since the function transferOwnership(), that was inherited from Ownable, requires the same thing:
require(owner_ != address(0), "Swap::constructor: Owner must not be 0");
This require statement increase gas usage for no reason.
Manual code review.
Remove this require statement.
#0 - Shadowfiend
2021-10-29T21:12:22Z
Will likely be nuking this entirely due to #78 .
🌟 Selected for report: pants
79.974 USDC - $79.97
pants
The modifier EmergencyGovernable.onlyTimelockOrEmergencyGovernance() calls governor() twice, resulting with two SLOADs operations for the _owner state variable that was inherited from Ownable.
Storage reads are much more expensive than reading local variables.
Manual code review.
Call governor() only once and cache the return value in a local variable. Then, use the local variable to load this value again.
#0 - Shadowfiend
2021-10-29T21:14:39Z
May or may not update this, depending on available time.
🌟 Selected for report: harleythedog
Also found by: pants
35.9883 USDC - $35.99
pants
This line in Swap.setFeeRecipient() perfoms an SLOAD operation for a value that is already stored in a local variable:
emit NewFeeRecipient(feeRecipient);
Storage reads are much more expensive than reading local variables.
Manual code review.
Use the already existing local variable instead of loading this value from storage:
emit NewFeeRecipient(feeRecipient_);
#0 - Shadowfiend
2021-11-03T21:49:26Z
Duplicate of #19.
🌟 Selected for report: pants
79.974 USDC - $79.97
pants
At both lines 251, 253 at Swap.sol you refer token[i]. You can cache it (address token; ... token = tokens[i];) and use the cached value instead. This is the gas efficient way to deal with double reading of such variable.