search

code-423n4 / 2021-10-tally-findings

0 stars 0 forks source link

Users can avoid paying fees for ETH swaps #68

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

pants

Vulnerability details

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.

Impact

Users can execute ETH swap without paying swap fees for the ETH the got from the swap.

Proof of Concept

The steps of the attack are:

  1. Deploy a malicious contract (denoted by M), that will be used for zrxBuyTokenAddress.
  2. Call Swap.swapByQuote() with zrxBuyTokenAddress=M and minimumAmountReceived=0. The rest of the arguments should specify our ETH swap, nothing special here.
  3. Define M to return 0 and 1 at the first and second times when fillZrxQuote calls zrxBuyTokenAddress.balanceOf(address(this)), respectively.
  4. As a result, 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.

Tool Used

Manual code review.

Recommended Mitigation Steps

Charge swap fees for the "refunded ETH" on ERC20 swaps (when boughtERC20Amount > 0), or require boughtETHAmount == 0.

Shadowfiend commented 2 years ago

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.

0xean commented 2 years ago

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.