Selecting a safe nonce that is not the same as the on chain safe nonce results in contract signature reverts

[habdelra]

We are using a relay server that experienced a chain reorg, and a bunch of the safe txns persisted to the DB had nonces that were out of sync with the main trunk of the network. This resulted in subsequent "GS026" reverts in the GnosisSafe contract which is the revert that you encounter when ecrecover fails to resolve the safe owner correctly. We believe this happened because the nonce that is used in the signature check for the GnosisSafe is the on-chain nonce for the gnosis safe, which did not match the nonce used that was signed which is based on the TransactionService.estimate_tx, who in turn uses the TransactionService.get_last_used_nonce method. https://github.com/gnosis/safe-relay-service/blob/master/safe_relay_service/relay/services/transaction_service.py#L244-L255

Specifically TransactionService.get_last_used_nonce looks like this:

    def get_last_used_nonce(self, safe_address: str) -> Optional[int]:
        safe = Safe(safe_address, self.ethereum_client)
        last_used_nonce = SafeMultisigTx.objects.get_last_nonce_for_safe(safe_address)
        last_used_nonce = last_used_nonce if last_used_nonce is not None else -1
        try:
            blockchain_nonce = safe.retrieve_nonce()
            last_used_nonce = max(last_used_nonce, blockchain_nonce - 1)
            if last_used_nonce < 0:  # There's no last_used_nonce
                last_used_nonce = None
            return last_used_nonce
        except BadFunctionCallOutput:  # If Safe does not exist
            raise SafeDoesNotExist(f"Safe={safe_address} does not exist")

In this function we are comparing the nonce cached in the DB (SafeMultisigTx) with the nonce that is used on chain from the GnosisSafe contract--and if they are different we use the largest one. I think this is actually incorrect. If you choose a nonce that is not exactly what matches onchain in the GnosisSafe contract then you will fail the checkSignatures() logic in the GnosisSafe contract. The GnosisSafe contract actually uses the on-chain nonce when constructing that data hash to use in ecrecover https://github.com/gnosis/safe-contracts/blob/main/contracts/GnosisSafe.sol#L126-L141. Providing a nonce that is different than this will result in failed signature checks.

I think get_last_used_nonce needs to be updated to just always use the on-chain nonce for the safe.

[Uxio0]

I think get_last_used_nonce needs to be updated to just always use the on-chain nonce for the safe.

At the beginning we were doing that, but when a client sends a transaction to the relay, while it's being mined they were expecting to get the next nonce and not the previous one, that's why we take into account the transactions that were send but not executed yet.

I think in your case your best solution is to empty redis and clean not executed transactions from SafeMultisigTx table