Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Apps Tx labeled as "Contract Interaction" if executed too fast #1206

Closed
francovenica opened this issue Aug 4, 2020 · 9 comments
Closed

Apps Tx labeled as "Contract Interaction" if executed too fast #1206

francovenica opened this issue Aug 4, 2020 · 9 comments
Assignees
Labels
Bug 🐛 Something isn't working Updated The bug is still reproducible

Comments

@francovenica
Copy link
Contributor

francovenica commented Aug 4, 2020

ENV: https://safe-team.dev.gnosisdev.com/app/#/safes/0x5D0edD9272a653Bf6b3b49e9856c7030942d2003/transactions

Steps:
Have a safe with 1 owner and tokens (ETH, DAI)
Use an app, like compound and lock a few tokens first
Now, try to lock and withdraw more tokens, as quickly as possible: As soon as you end signing one tx create another and sign it

You will see that the compound apps, instead of being labeled as "Compound" in the tx they will be labeled as "Contract interactions"

Chances are the regular users will not do this, but it would be good to know why creating and signing tx really quick causes this issue.

This issue doesn't happen if you take time between tx and tx

image.png

@francovenica francovenica added the Bug 🐛 Something isn't working label Aug 4, 2020
@lukasschor lukasschor added the Major Needs to be fixed for immediate next public release. label Aug 6, 2020
@tschubotz tschubotz added Minor Needs to be fixed within the next 1-3 public releases. and removed Major Needs to be fixed for immediate next public release. labels Nov 25, 2020
@katspaugh
Copy link
Member

Still an issue. For me, only the first transaction was marked as Compound. The subsequent ones were just a "Contract interaction".

@katspaugh katspaugh added the Updated The bug is still reproducible label Apr 7, 2021
@tschubotz tschubotz added Major Needs to be fixed for immediate next public release. and removed Minor Needs to be fixed within the next 1-3 public releases. labels Apr 28, 2021
@nicosampler nicosampler self-assigned this May 4, 2021
@nicosampler
Copy link
Contributor

This issue is somehow related to this other: safe-global/safe-smart-account#187.

I've talked with @rmeissner and at the moment, there is nothing or at least nothing simple we can do about it.

As any user has a complaint about it, we will not give priority for now.

CC: @tschubotz @francovenica

@nicosampler nicosampler removed the Major Needs to be fixed for immediate next public release. label May 5, 2021
@tschubotz
Copy link
Member

This issue is somehow related to this other: gnosis/safe-contracts#187.

Could you please elaborate on how exactly it is related to this issue on the contracts?

My high level understanding would be that this "safe apps" marker is something that the frontend sets, independently from the type of txs or the number of owners, no? So how is it related to low-level contract logic? 🤔

@nicosampler
Copy link
Contributor

sure, sorry, we had a chat on slack with the details. I'll try to clarify them here:

When a user creates a tx, in the frontend, we are setting the nonce and the tx-data. If the user starts creating txs one after the other without giving some time to the backend to "index" the new tx there will be a bunch of txs sent with the same nonce. That is because the frontend is relying on the backend and the contract to get the next nonce. As none of them was updated (yet) we are sending the same nonce until the backend or the contract is updated.

@tschubotz is it more clear?

@tschubotz
Copy link
Member

@tschubotz is it more clear?

Now I got it, thanks. So these txs that Franco was posting would eventuelly all fail?

I actually just tried again. And I was unable to reproduce. If Franco is fine, I think we can close this one then, yes.
image

@nicosampler
Copy link
Contributor

no, they are executed anyways for the case of threshold = 1. The contract assigns them the next nonce.
Check this example: https://safe-client.rinkeby.staging.gnosisdev.com/v1/safes/0xb183e0f1dAC5606A479Eb39633C9E25Cb9273885/transactions/history/

@tschubotz
Copy link
Member

tschubotz commented May 5, 2021

The contract assigns them the next nonce.

It's not that the contract "assigns" a nonce. The user signs this specific nonce in the Metamask popup, right?

So you are saying that:

  • For making the txs and the Metamask popup, the correct Safe nonce is used.
  • For letting the backend know about the Safe app info (origin) the wrong nonce is used?

Is this correct?

Why this discrepancy?

@rmeissner
Copy link
Member

rmeissner commented May 5, 2021

Our interface uses the "wrong" nonce in all cases (MM and service). But if you rely on msg.sender and have a threshold of one the Safe contract don't check the nonce, therefore the tx doesn't fail with the "wrong" nonce (see the issue @nicosampler linked: safe-global/safe-smart-account#187).

@tschubotz
Copy link
Member

Aah, now I got it, thanks for explaining. 🙏
Well then, let's close this as won't fix for now then!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug 🐛 Something isn't working Updated The bug is still reproducible
Projects
None yet
Development

No branches or pull requests

6 participants