Possible replay attack

[christoph2806]

From a reddit post of @naterush:
The multi-sig specified allows failed transactions to be replayed.

To see why, check out this line. If the sub-call is not successful, the multi-sig will throw. This means the nonce is not updated if the sub-call throws, reverts, or runs out of gas.

Thus, all failed transactions can be replayed by anyone in the world (just by watching the multi-sig) at any point in time, up until the next successful transaction is executed by this multisig. This very much defies the expectations we come to expect from an Ethereum tx.

[coder5876]

@christoph2806

Yes, I’ve discussed with Nate about this. It’s a hard problem to solve since we’re moving the nonce logic from the transaction level up to the smart contract level where transaction failure ideally should have no impact on contract state.

One suggested remedy is to not catch the error in the call and have a failing transaction technically succeed and let the nonce update. This could however lead to an IMO worse attack, where an attacker could front run a transaction with high gas price and low gas limit, leading the transaction to fail and blocking the legitimate transaction.

[christoph2806]

1, Frontrunning could be avoided if you change the contract to some variant of an "owned" contract, allowing only the owners to submit a transaction.
2. Even without so, what could an attacker possibly do apart from speeding up the tx? The "attacker" must have a collection of valid signatures of owners with the correct nonce. But in this case, it's hard to say that he's an "attacker". Or am I wrong with my reasoning?

[coder5876]

@christoph2806

what could an attacker possibly do?

The main thing is a DoS attack: you try sending a transaction, the attacker sends a transaction with the same data packet, lower gas limit and higher gas price. The attackers transaction gets picked up first by miners but fails due to the low gas limit. However the nonce is increased and so the original transaction is no longer valid and the signers need to sign again with an increased nonce.

[christoph2806]

Possible mitigation:

• encode the gas limit in the tx hash together with nonce.
• check for enough gas beforre increasing nonce.

[CBDotGuru]

Found out the execute call is close to the stack depth limit. Two things:

For this to be universal, I think cross chain replay attacks should be considered as brought up here in the ERC-191 discussion. I proposed a mitigation toward the bottom of that discussion chain. In that proposal, adding the chainId gave me a stack depth issue with this wallet so I had to combine the first two bytes.

With that said, adding any more params doesn't seem feasible. I elected to go with requiring a call from one of the key holders. That's easy to enforce with 3 sigs, but more than that it could get ugly really fast.

[ScJa]

I'm a bit late to the discussion, but I think increasing the nonce on failure is valid approach if done correctly. I think @christoph2806 was talking about only increasing the nonce if the sub-call fails, while @christianlundkvist thought to increase the nonce on every failed transaction (even if the signature validation fails).

If we would change the code to something like this:

     nonce = nonce + 1;
     if (!destination.call.value(value)(data)) {
           // here you log the failure via an event or introduce a storage variable to save failed transactions
     }

It would be impossible by for someone external who does not have enough signatures to front-run and increase the nonce. However if a transaction is called by the owners and fails, the nonce will increase.

This changes the functionality of the execute-function slightly but I think it should both avoid replay-attacks and front-running. Please let me know if I overlooked anything.

[ScJa]

I just thought of another possiblity how the replay attack could be relevant. If the owners of the wallet accidentally sign a transaction with a future nonce.
Attackers technically wouldn't know that this is the case, but if they would try to execute the transaction after the next few transaction it might succeed.

To fix this a random nonce would be needed I think. Incrementing on failure might even be counter productive.

[coder5876]

@ScJa The front-running issue doesn't require the attacker to be able to create signatures, they would simply grab the already signatures from a transaction in the mempool and send them in a second transaction with higher gas price. This second transaction is more likely to be picked up and if it fails will block the original transaction.

[ScJa]

@christianlundkvist Thanks for the answer. I overlooked the possibility for the attacker frontrunning with just enough gas to reach the sub-call and then running out of it, my bad. Seems like a tricky situation overall.

The only clear fix I have in mind now is to include the message sender in the signatures.

[coder5876]

I'm leaning towards erring on the side of simplicity here: The mitigations against replay attacks will increase complexity of the contract, and I think the risks are relatively niche. You could also imagine mitigations where your frontend checks if you have outstanding failed transactions and asks you to send a dummy transaction to get rid of the replayable failed transaction.

Furthermore I think with the upcoming account abstractions we may be able to have the best of both worlds here with multisig contracts that acts more like accounts.

[ScJa]

I agree with your points, it would be really hard to abuse the vulnerability in a meaningful way. I will also take a look at account abstractions, it definitely looks interesting and I haven't heard of them yet.

Apart from that, I don't believe a fix by including the sender in the signature would increase complexity (at least code-wise), but it will reduce the usability slightly.
Correct me if I'm wrong but I think to eliminate the frontrunning issue you would only have to add the msg.sender to line 29 like so:

bytes32 txHash = keccak256(byte(0x19), byte(0), this, msg.sender, destination, value, data, nonce);

[naterush]

That works (and this is the approach uPort took in their Meta Transactions), but it requires an assumption that the relayer will do the right thing (and not send 2 little gas).

Ideally, we wouldn't have to make an assumption about the honesty of the relayer (though it's pretty minor, realistically), but this requires a more significant complexity tradeoff like including the amount of gas in the signature.

[coder5876]

In v2.0.0 we've addressed this by including the option of specifying an executor to execute a transaction. I feel that this should take care of most replay attack concerns.

[JhonnyJason]

Hahaha just followd through this discussion @christianlundkvist - nice one - remarkably simple and effective Solution then in the end ;-)