sygmaprotocol / sygma-widget

Transfer widget for the sygmaprotocol
5 stars 2 forks source link

Allow Spending inclemently does not calculate the correct Allowed amount #121

Closed LyonSsS closed 3 months ago

LyonSsS commented 4 months ago

Severity: Low | Priority: Medium

Note: Inclemently increase the allow spending amount does not calculate correctly the total allowed amount to be spent.

Prec: Have a wallet with funds on Sepolia ( ex 100 ERC20TRLTEST token)

STR:

  1. Select from Sepolia and connect metamaks.
  2. Select To as Mumbai.
  3. Select the ERC20 drop down for ERC20TRLTEST token and insert amount of 10 to be transferred.
  4. Select Allow Spending and sign form metamask.
  5. After TX is validated, increase to 11 ERC20TRLTEST to be transferred and allow spending for 11 tokens from Metmask.
  6. After TX is validated, increase to 12 ERC20TRLTEST to be transferred

Exp: The UI not to prompt allow spending if the amount to be transferred is <= 21 Act: A new tx for allow spending is prompted to metmask.

MakMuftic commented 4 months ago

Hey team! Please add your planning poker estimate with Zenhub @Lykhoyda @mpetrunic @wainola

mpetrunic commented 4 months ago

@LyonSsS Just to make this more clear. Issue is that amount validation doesn't take fee into consideration when validating?

LyonSsS commented 4 months ago

The issue is easy to be replicated by following the STR provided on the ticket. If I consecutively allow spending to amount 10 of token type A, then in UI incrementing to 11 Amount of token type A ... I get a request to allow spending of another 11 amount ( and I allow it) ... then when I consecutively increment to 12 token type A in UI ... I should be allowed to transfer ... as I already allowed spending for 10 + 11 = 21 token of type A ... which makes it over 12 amount. This is not regarding fee. It is about making the right calculation of the allowed to be spend amount, after we did it in two consecutive TXs. CC: @mpetrunic

mpetrunic commented 4 months ago

@LyonSsS Approvals are not adding. If you first approve 11 then in second transaction approve 12. You only have approved 12 (not 23). Each approval overwrites old value: https://eips.ethereum.org/EIPS/eip-20#approve

If you instead approve 100 (by changing value in metamask) it won't prompt you again.

But related to this I think we have different kind of bug which I thought this issue is about. If you have only 10 tokens. Bridge fee is 1token and you set 10 in amount field it will pass the validation because validation doesn't take fee into account.

I would close this issue and open another one regarding not taking fee into validation

LyonSsS commented 4 months ago

@mpetrunic ah I had different understanding for the Amount approved. In this case ... this issue is out of scope and I will close it.

"But related to this I think we have different kind of bug which I thought this issue is about. If you have only 10 tokens. Bridge fee is 1token and you set 10 in amount field it will pass the validation because validation doesn't take fee into account." - this is not ideal. But be careful as the methods when sending from Substrate has a different logic from the one that sends from EVM. EVM - > used total + fee / Substrate deducts FEE from total (I mention the substrate for the UI logic flow if it impacts fee). Note: EVM fee for tokens might be with Fixed Fee or Percentage Fee ( implies a min, a max and % * Amount for the in between min/max value). Just to have a generic design from the beginning.

This is not an issue.

mpetrunic commented 4 months ago

@LyonSsS I don't think right now we are substracting fee from amount user inputed.

If you input 10TOKEN, we will actually take 11TOKENS (10trasnfer + 1 fee) from your account. If we keep current flow we should update validation to take that into account (need to ensure you actually have 11 TOKENS).

Personally, I think, we should change flow (it's easier) to subtract fee from amount.

LyonSsS commented 4 months ago

@mpetrunic agree. I think this input field validation, it is co dependent on fee ( plus allow spending), makes more sense to be updated once we display the fee in flow.

mpetrunic commented 3 months ago

closed in favour of #139