stdStorage: support packed variables

[klkvr]

This was initially designed to fix probles with USDC, but it seems that we are now able to modify pretty much any packed storage variables.

This is still a WIP as I believe binary operations can be rewritten a bit cleaner but it already works.

I've updated some of the negative tests on failing packed variables decoding to being positive.

The solution is to try finding such offsetLeft and offsetRight for a storage slot, that it's value[offsetLeft:-offsetRight] exactly equals the return data of the given function

[mds1]

Two comments, though I have not reviewed yet, and cc @brockelmore in case he has any thoughts:

1. feat(stdStorage): Allow writing to a packed storage slot #148 was an attempt at this, but you'll see there is a failing test which we never got around to solving
2. What do you think about moving this functionality upstream to rust, as part of feat: move cheatcodes from forge-std to forge foundry#3782?

[klkvr]

@mds1

1. Haven't noticed that PR. I will add and look into this failing test
2. Yes, it definitely makes sense to move this logic to native cheats, however, there are some blockers for making calls to EVM from cheats. This issue looks important, so temporary solution on the forge-std level makes sense imo

[klkvr]

hey @mds1 after some updates I managed to make that test pass.

Do you think that it makes sense to make packed variables discovery optional as it's done in #148 via enabledPackedSlots? The reasoning about it cluttering call trace makes sense, however on the other side users having troubles with USDC will be able to fix tests by simply upgrading forge-std if it will be enabled by default

[mds1]

Is there a way we can special case these traces upstream in forge to hide them from output, or condense them into a one-line summary of what's been hidden? If not, I do think we should make it opt-in, because the size of the trace can be pretty dominating compared to the rest of the trace

[klkvr]

I don't think that we have a way to hide specific traces. I'll make it optional for now, and will keep that in mind for the moment when we'll be upstreaming stdStorage to cheats

[brockelmore]

I will take a look but i thought about doing the algorithm laid out in this paper: https://seclab.cs.ucsb.edu/files/publications/ruaro24crush.pdf

basically: you track sloads’ masks and shifts, then when an sload is returned you’re given the offset right and left. this would be a modification of a cheatcode (or an entirely new cheatcode). Likely this would look like watching for an sload, tracking its spot in the stack, and any mask or shifting operations would be recorded. Upon a return, the left and right offset would be calculated and returned. Inside StdStorage, these offsets would be stored and applied on a write.

[klkvr]

@mds1 it seems that this fixes the USDC issues even with enable_packed_slots disabled

this is happening because of the overall change of slot discovery logic

previously it was working like this:

• If call triggers only one storage read and value of read slot matches return value, slot is used
• If call triggers more than one storage reads, then used first slot such that it's value matches return value, and placing ~slot_value into it results in return value being equal to ~slot_value

The new impl always uses first slot such that it affects return value (checked via checkSlotMutatesCall) and it's value matches return value.

New impl is safe to use for writing as we only support "checked" writes which verify that resulted slot value matches the value that we wanted to write to it.

However, in some cases new impl might be bad at reading as it may mistakenly find packed slot. Previous impl could do that too for single read case. I don't think that using StdStorage.read is really a common usecase and we are not introducing breaking changes here, so it should be fine imo

[mds1]

Oh cool, that's great that it fixes the USDC issues even with enable_packed_slots disabled. The main thing I want to confirm is: in the USDC contract storage do we end up overwriting the blacklisted bit if it's set, or do we preserve the value of unrelated bits? (Same question more broadly too, just using USDC as an example).

Also, given that the USDC case works even with enable_packed_slots disabled, what exactly changes when that's enabled?

And admittedly I have not closely read the code in this PR yet so unfamiliar with the details which may answer these questions

[klkvr]

in the USDC contract storage do we end up overwriting the blacklisted bit if it's set

in that case slot value will not match call return value and find will fail.

what exactly changes when that's enabled

when it's enabled we do not require slot value to exactly match call result, but instead are looking for a slot bits range (basically pair of offsets) that does match it

[mds1]

Thank you!