test: Intent's static + dynamic offsets' field corruptions

[smitrajput]

Test Description

This is a good first step for issues 201 and 212, testing potentially unsafe decoding by corrupting the 13 static fields and offsets of all 7 dynamic fields of Intent struct, along with Intent's main offset, during calls to Orchestrator.execute(), targeting naked decoding at _extractIntent(), selfCallPayVerifyCall537021665(), _extractPreCall() and IthacaAccount.pay().

Tests are split into 21 functions: 13 for corrupting static fields, 7 for dynamic field offsets and 1 corrupting the main intent offset.

Result

All tests pass clean without the corruption (base case in existing tests), and throw EvmError: Revert after corruption for 6 dynamic field offset corruptions as expected, caught as false value returned on doing a .call() to Orchestrator.execute(). The remaining 15 return well-defined errors in the codebase which are caught neatly. Implying corrupted offsets are unable to induce unintended behaviours and the decoding throughout the call from Orchestrator.execute() to IthacaAccount.pay() is safe from this attack.

Potential Improvement

Would have preferred a simpler multichain intent setup, to test the corrupted settlerContext field than the current one, which reuses most of Orchestrator.t.sol's testMultichainIntent() setup. Improvement suggestions welcomed.

Future Work

Note that tests for corrupting the data of dynamic fields still remain. Worth adding in a future PR.

[smitrajput]

@legion2002 @howydev plz take a look and lemme know of improvements

[legion2002]

@smitrajput Thanks we'll look into it soon, these tests will be very helpful!

[smitrajput]

Looking forward to the review @legion2002!
Have a bunch more tests we can add once the current ones are approved:

[howydev]

we don't use console.logs in our tests, could you leave these as comments instead?

[howydev]

also do this for the rest of the console logs, please

[smitrajput]

fixed

[howydev]

nit: remove this

I assume this doesn't pass because it reverts with a generic EVM error?

[smitrajput]

removed.

yes, this is the EvmError: Revert error we get on corrupting every dynamic field offset

[howydev]

I'm not sure if works

If dataPtr is an encoded intent, wouldn't add(maliciousCalldata, 0x20) point to intent.eoa?

[smitrajput]

no, as mentioned in that comment, add(maliciousCalldata, 0x20) points to the main offset of the Intent struct, as the first 32 bytes of maliciousCalldata store the length of the encoded Intent struct. Plz check out the calldata layout here, to visualise it. I've also simplified the yul there, for easier understanding.

[howydev]

could you think of a better check here instead of a generic execution failure? im a little worried that some of these tests are failing not because of corrupted calldata, so the test is inaccurate

[smitrajput]

managed to add better checks for 2 (Test 2 and Test 6) of the 8 dynamic offset corruptions, but couldn't find anything significant for the remaining 6 as they either

1. fail and return the generic EvmError: Revert on corrupting with out-of-bound values like 2^64, with a return value 0x, so further try/catching or error logging doesn't help
2. or pass and return 0x00000000 with within bounds values like 0x300, 0a20
   
   
   

Also note that these tests are indeed failing because of offset corruption, as I corrupted all the 13 other static fields and we're seeing defined errors being thrown and caught for all of them.

[howydev]

hi there, thanks for making this submission!

i'm slightly worried that some of these tests are not checking what they're supposed to be checking. would love if there's a way to do a check other than a generic execution failure, since there are a lot of reasons that error would happen

(for instance, i suspect that for one of the tests we're switching intent.eoa to a different eoa. in this case the execution call would fail in a similar way)

[smitrajput]

thanks a lot for the review @howydev

I double checked to see if the offset corruption is correct, and we are indeed corrupting the exact offsets. To help with the calldata corruption visualisation, plz check out this calldata layout in the tests, so we're sure we're corrupting the correct offsets.

Also added better checks for 2 (Test 2 and Test 6) of the 8 dynamic offset corruptions, but couldn't find anything significant for the remaining 6 as they either

1. fail and return the generic EvmError: Revert on corrupting with out-of-bound values like 2^64, with a return value 0x, so further try/catching or error logging doesn't help
2. or pass and return 0x00000000 with within bounds values like 0x300, 0a20
   
   
   
   

To further prove that we're not getting those generic reverts bcoz of any adjacent fields' corruption (or any other random reason), I corrupted all the remaining 13 static fields and we're throwing and catching well-defined errors for all of them. Plz find those tests here.

This proves that the execution call fails differently for dynamic offsets corruption and static fields corruption.

You make a valid point that the generic EvmError: Revert might pop up for various other reasons, but by observing all those reasons (in this case the adjacent static fields' corruption) to be failing for errors other than EvmError: Revert, we can safely say that all the remaining 6 EvmError: Reverts are indeed thrown by the 6 dynamic field offsets corruptions.

[howydev]

hey, apologies for the delay, didnt see that you replied

will allocate some time later this week for a review

[howydev]

hey @smitrajput - really appreciate the time taken for this review, but we're likely going to switch to formats that remove all calldata offsets, using packed mode encoding instead of encoding a struct

By removing offsets, we kill 2 birds with a single stone - reduce the cost of operations, and no more security issues from custom abi encodings

[smitrajput]

that's a cool idea @howydev, curious to have a look at the updated implementation and see if I can reuse these tests (at least the static fields corruption part and add the missing dynamic data corruption part too), to check the new and different security implications of the packed encoding

[howydev]

changes are ready: #365

instead of having a struct that comes with offsets, we have a custom encoding for our bytes instead. without the offsets it's easier to reason about the security and we save 12k in gas when we do this, so we'll likely opt for some version of this instead of the original struct

would love your eyes on this if you have spare time, and if you think of any tests we should add feel free to propose!

[smitrajput]

sound!

taking a look asap