feat: add simple anti front running fix #574
Conversation
|
A question, why sha the address instead of checking address directly? |
| @@ -74,6 +74,8 @@ library PaymentChallengeIFENotCanonical { | |||
| ) | |||
| public | |||
| { | |||
| require(args.senderData == keccak256(abi.encodePacked(msg.sender)), "Incorrect senderData"); | |||
There was a problem hiding this comment.
doesn't the "respond to non-canonical challenge" (not sure of the exact name) require one as well?
There was a problem hiding this comment.
Hmm, good point. But maybe neither "challengeInFlightExitNotCanonical" nor "respondToNonCanonicalChallenge" need this protection right now.
That's because the IFE bond doesn't get paid out on a canonical challenge or response - the owner of the bond is changed here, but the bond doesn't get paid out until processExit is called.
So while a front runner could potentially benefit by front running a canonical challenge or response, they would have to be very familiar with how the exit game works.
Um. I'm inclined to add it in anyway though, for consistency with the other methods, and possible an easier path to adding more sophisticated front running protection later.
What do you think?
There was a problem hiding this comment.
ah you're right! I'd definitely vote for consistent, to not surprise our future selves reading the code.
Just the question now is - what will be consistent:
senderAddresson anything resembling a challenge and related to any bond operation orsenderAddresson anything that actually can pay anything out.
On 2nd thought, the latter makes more sense, so it seems it should be dropped from challengeIFENotCanon.. The downside I see is that if sometime, someone decides that those call should pay some portion of the bond (for whatever reason), they'd have to remember to do senderAddress check.
Not 100% sure here.
There was a problem hiding this comment.
I'm going with option 2; only putting senderData on anything that actually pays out.
Trusting our future selves to do the right thing if or when we implement a more sophisticated front running solution
| @@ -59,6 +59,7 @@ library PaymentInFlightExitRouterArgs { | |||
| uint256 competingTxPos; | |||
| bytes competingTxInclusionProof; | |||
| bytes competingTxWitness; | |||
| bytes32 senderData; | |||
There was a problem hiding this comment.
Do the integration docs and the .md files documenting the contracts reflect this change?
There was a problem hiding this comment.
Yep, will update them
| @@ -547,7 +547,8 @@ PaymentExitGame.challengeStandardExit({ | |||
| exitingTx, | |||
| challengeTx, | |||
| inputIndex, | |||
| witness | |||
| witness, | |||
| spenderData | |||
There was a problem hiding this comment.
senderData? (multiple occurrences)
|
Wow, sorry @boolafish - I typed out an answer for this last week, but it must not have got sent correctly 😞
It's to (slightly) obfuscate the sender address, so that a script won't simply replace it with their own address |
|
Dammit, I had a longer description as well, but it got lost too! 😠 I'll try to remember what I wrote originally... |
|
hm, sorry, I could swear I did an approve ✔️ when placing that comment about |
You had approved @pdobacz , but the later commits forced a re-review 😄 |
Simple front running protection.
Challenger has to provide data derived from sender, so that simply re-submitting the tx with a higher gas price doesn't work
https://github.com/omisego/security-issues/issues/11
There have been 3 options proposed:
hash(txbytes || nonce || sender) = "0x00000000" <> anythingthen contract code would just compute additional hash and check required zeros
Option 1 is obviously the simplest to implement for both the contracts and the callers, and should defeat most generic front running scripts. Defeating front runners who are specifically targeting challenges will probably require more thought.