Safe-react: The Execute button is active but not all confirmations are sent

[katspaugh]

Bug description

Reported by @Georgi87:

I tried to add an owner and all 3 required owners confirmed without executing it. This happened as the interface was not updating fast enough to show the confirm and execute option. As you see the interface showed this strange state: 3/3 confirmed but in the UI it still said it would wait for two more confirmations but the execute button is still shown. When clicking execute MetaMask didn't show any gas amount information. When setting the gas manually to 300 and executing it the transaction failed with the error signature data too short. Ultimately we solved it by rejecting the transaction and submitting it again.

this is the failed submission with manually set gaslimit to 300k.

Screenshot

Tx details

https://etherscan.io/tx/0x383a1aa69acb9f7f741f8bd7fa54ba3a5bd1f80a3c62a2141b6e2c0c3b8e6e16

Env

they were all hardware wallets connected via metamask

Possible causes

@fernandomg:

from what I understand, the inconsistency between the buttons status and the owner's list is due to an out-of-sync between the data returned by the gateway-client endpoints.
one is for the general view (/safes/{safe_address}/transactions/queued)
the other is for tx details (/transactions/multisig_{safe_address}_{safeTxHash})
The buttons status is based on what the general endpoint returns
The list of owners is based on what the detailed endpoint returns

note: To build the tx, the signatures are requested to the detailed endpoint.

then, we should check what the detailed endpoint returns before triggering the tx execution

Solution

@rmeissner:

Even if the data from the service is "wrong" we should never miss a signature when sending it to MM

[tschubotz]

Based on @fernandomg 's comment, it seems like the client gateway (CGW) endpoint for the tx list returned 3/3 signatures whereas the CGW endpoint for the tx details only had 1.

I couldn't reproduce this end-to-end right out of the box, i.e. I couldn't get the GGW to have these inconsistent states.
However I tested the frontend based on @rmeissner 's comment

Even if the data from the service is "wrong" we should never miss a signature when sending it to MM

Here's what I did:

Attempt 1

• Have a Safe with threshold 3 and 3 owners.
• Create tx with owner A
• Block via Chrome dev tools the requests to the CGW tx details endpoint for that specific tx.
• Sign but don't execute with owner B, C
• Owner A now has the exact same state like in Stefan's screenshot, i.e. it says "3/3" on the list, "execute" button is displayed but on the tx details, there is only 1 owner that signed apparently.
• Click execute
• Nothing happens, since the frontend seems to pull info about this tx from the CGW again which is blocked.

So, Attempt 2 with a proxy:

• Have a Safe with threshold 3 and 3 owners.
• Create tx with owner A
• Intercept requests to the CGW tx details endpoint for that specific tx via a proxy (I used mitmproxy)
• Store json for when only 1 owner signed.
• Sign but don't execute with owner B, C
• Owner A now has the exact same state like in Stefan's screenshot, i.e. it says "3/3" on the list, "execute" button is displayed but on the tx details, there is only 1 owner that signed apparently.
• Click execute
• Via the proxy, now edit the CGW response to only contain 1 signature
• Metamask pops up
• Gas limit is 0 -> sign that it couldn't get estimated
• Set gas limit manually to 300k in Metamask
• Submit tx via Metamask
  -> Tx fails (here's mine)

So, even though I couldn't get the CGW in this inconsistent state, I think the frontend shouldn't trigger invalid transactions with not enough signatures :)