search

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

0 stars 0 forks source link

frontrun swapByQuote or abuse high allowance - replacement #17

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

gpersoon

Vulnerability details

Vulnerability details

Note this is a replacement for the previous sumitted issue (additional steps are required for abuse)

Impact

The normal flow for swap transactions is: 1) user gives swap contract allowance for "inputtoken" 2) user calls swap contract (which performs the swap)

Suppose the user has gives a large (unlimited) allowance, which stays present after a swap.

Then you could formulate a swapByQuote() with a hacker-contract as parameter and sweep the tokens for which the user still has an allowance, provided you can trick the user to sign this transaction. Tricking the user to sign the transaction requires additional social engineering in a malicious website. Note: it is possible to retrieve from the chain which users have open allowances. Note: a theoretical risk is also frontrunning transaction#2 of the user with a malicious transaction; this is probably more difficult to create.

See proof of concept:

Proof of Concept

Create a hacker_token or use any other cheap token

Create a hacker_contract with following content:

function hackerfunction(amount) {
  IERC20(inputtoken).safeTransferFrom(msg.sender, address(this), amount); // retrieve input tokens from msg.sender, which is the swap contract
  IERC20(hacker_token).safeTransfer(msg.sender, 1); // give back some hacker_tokens, so swap contract is happy
  return true;
}

Call function swapByQuote() in the following way: swapByQuote(inputtoken,amount,hacker-token,1,1,hacker_contract,hacker_contract,hackerfunction&amount,block.timestamp+100) Where:

swapByQuote() gives an allowance to hacker_contract swapByQuote() calls fillZrxQuote(), which calls hacker_contract The hacker_contract now has an allowance and can transfer the tokens from the swap contract. It should give back a token to make sure the logic in the swap contract continues.

https://github.com/code-423n4/2021-10-tally/blob/c585c214edb58486e0564cb53d87e4831959c08b/contracts/swap/Swap.sol#L106-L186

https://github.com/code-423n4/2021-10-tally/blob/c585c214edb58486e0564cb53d87e4831959c08b/contracts/swap/Swap.sol#L212

Tools Used

Recommended Mitigation Steps

Make sure the allowance by the user to the swap contract is as low as possible and/or is set to 0 after the call to swapByQuote() Use a frontrun resistant RPC to submit transactions Have a white list for allowed zrxTo addresses

Shadowfiend commented 3 years ago

Going to mark #1 as a duplicate of this, since that seems to be the intent in the description.

Ultimately I think we'll acknowledge here, but unsure if the severity is correct. “The user may be tricked into signing their tokens away” is generally true, and is not really in our threat model for the contract itself. The contract is meant to be fed by the 0x API, via a UI that handles the 0x API interactions, so I don't know that we're currently super-concerned about this scenario.

0xean commented 3 years ago

"Tricking the user to sign the transaction requires additional social engineering in a malicious website."

This to me is a systemic risk in blockchain, not specific to this protocol. If I can trick you into signing a transaction, I can always steal all your funds.

Per the judging documentation

Unless there is something uniquely novel created by combining vectors, most submissions regarding vulnerabilities that are inherent to a particular system or the Ethereum network as a whole should be considered non-critical. Examples of such vulnerabilities include front running, sandwich attacks, and MEV. In such events, leave a comment on the issue:

I am therefore marking this as non-critical since the premise is "I can trick you to sign a transaction".