search

MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.91k stars 4.87k forks source link

Issue with spend limit permissions - MakerDao voting contract #8146

Closed webs7er closed 4 years ago

webs7er commented 4 years ago

I signed the MakerDao voting contract while setting a max spend limit of "6.5". image (Note: screenshot is from another wallet)

When depositing to the contract, I could deposit MKR exceeding the pre-set limit, see tx: https://etherscan.io/tx/0x5559d3dd99a0442cd88195d4eef3193191a7f294861fe62eb8e6ebf50e5cba5f

On the other hand, when withdrawing from the voting contract, I could only take out up to "6.5" MKR.

  1. First tx failure (withdrawing max amount): https://etherscan.io/tx/0x0d875c835193088fa716f2d1fc930bfe11e9815039ca4e47bc12a0f4f7bc8d0c
  2. Withdrawing 6 MKR: https://etherscan.io/tx/0xcb00d7e0bbb098bc9860417d75995039eb81ccfb606966cc42937bb1e9d98342
  3. Withdrawing 0.5 MKR (for a total of 6.5): https://etherscan.io/tx/0x1a025b1a5c75d26f45b4a685cf39672f8b3739c1c93f80b29d4f843f8e50f922
  4. Attempting to withdraw the remaining 0.0693 MKR left in the voting contract: https://etherscan.io/tx/0x49725a42db9b5b932aacddc6d8a293bb92fa440df96b2996e1e98c5a2312ae38

To Reproduce:

  1. Go to https://vote.makerdao.com/ -> Voting Contract -> Vote with a single wallet
  2. Agree to the terms and conditions
  3. In the "Allow Vote.makerdao.com to spend your MKR?" MetaMask dialog, click "Edit Permission"
  4. Enter a custom spend limit, save and confirm the transaction
  5. In the voting contract, try to deposit MKR with value higher that the pre-set limit - the transaction goes through
  6. From the voting contract, try to withdraw MKR with value higher than the pre-set limit - the transaction fails image

Expected behavior From my understanding of the "spend limit" function, step 5. above should fail, whereas step 6. should succeed.

Browser details (please complete the following information):

Gudahtt commented 4 years ago

In the example failed transactions that you've shown, the functions being called are "lock" and "free" respectively. I'm not sure what these functions are supposed to do. They aren't ERC-20 functions, and we don't support them in MetaMask as far as I know.

I don't believe the screenshot shown would ever be shown for the transaction in question. Does that make sense? Let me know if you think I'm mistaken about any of this.

webs7er commented 4 years ago

Hi @Gudahtt, thanks for taking the time to look into this.

I am not quite familiar with the concept of limit permissions and I'm not sure what problem they are trying to solve. Personally, I believe they are not needed in this particular use case - although I did get the option to set a them (see Step 3. above).

In any case, I'd like to know what causes the withdrawal to fail (with the error 'ds-token-insufficient-approval') - is it something linked to the limits I set (and/or how I set them), or is there another issue elsewhere? Would it help if I try reaching out to the MakerDAO developers?

Gudahtt commented 4 years ago

is it something linked to the limits I set (and/or how I set them), or is there another issue elsewhere? Would it help if I try reaching out to the MakerDAO developers?

Yes, I think that would help. The failure you're asking about is specific to this contract (or type of contract), so the MakerDAO developers seem like the appropriate people to ask.

I'm going to close this issue for now, as I don't think there is any MetaMask bug here. But thanks for reporting this, and good luck in investigating this further!