daifoundation / maker-otc

The OasisDEX protocol - Simple on-chain market for ERC20 tokens
GNU Affero General Public License v3.0
101 stars 39 forks source link

Contract call fails and spends enormous amount of gas when attempting to trade against existing order #164

Open burdakovd opened 6 years ago

burdakovd commented 6 years ago

Scenario:

  1. I have 18.55 SAI (or so the UI said)
  2. I trade against existing big order to buy ETH for these SAI. I use "BUY MAX" button to populate amount of SAI I want to spend, and it populates it to 18.55
  3. The confirmation window says that it is going to use $69 in gas costs (but I didn't notice it initially)
  4. I confirm the transaction (thanks to Metamask being aware of too high gas limit I reduce the gas limit from 6M to 600k)
  5. Transaction fails without clear error message, and $5 in fees are waster
  6. Not knowing what is happening, I attempt trade multiple times, and $5 in fees are wasted multiple times

Transactions: https://etherscan.io/tx/0x2d8cf26141a73ecc6e3461d32928ada858cb7ccc2312c4c722405fe8df28b38d https://etherscan.io/tx/0xad98a83ea08c05921f4eee8c723b12356164735417b97d776f6dbf9aa3e49697 https://etherscan.io/tx/0x7310903aba9bfe6e14f4d5603a7204fc5c7b051071b6f69e7c434b7914fee35b

Here is what I think is happening:

When I manually change amount of SAI to spend to 18.54 (leaving 0.01 as buffer), transaction seems to succeed (still pending, but at least estimated gas cost and limit are reasonable): https://etherscan.io/tx/0xeb4f8fbef2322368a7eca3e8785193c4a2d5197598b66af93a1c102f7fc80ca3

Oasis UI (note gas cost): image

Metamask complaining: image

nmushegian commented 6 years ago

ABORT opcode didn't exist before byzantium. Leaving this issue open until we double-check that latest version won't drain your gas like this.

See this for context/history: https://github.com/ethereum/eips/issues/140

burdakovd commented 6 years ago

Just to clarify, I see three bugs here:

Do you agree these three bugs are in the scope for this issue?

nmushegian commented 6 years ago

This repository contains the on-chain contract code, I'm not too familiar with the rest and the issue should probably be split up. See: https://github.com/oasisdex