crytic / medusa

Parallelized, coverage-guided, mutational Solidity smart contract fuzzing, powered by go-ethereum
https://www.trailofbits.com/
GNU Affero General Public License v3.0
273 stars 33 forks source link

Fix a non-deterministic copy-length based panic in the parseBytes32 cheatcode #293

Closed Xenomega closed 4 months ago

Xenomega commented 4 months ago

The parseBytes32 cheatcode takes a string as input, casts it to a slice ([]byte), and then slices a 32-byte slice off of it, before performing a copy operation to the destination variable. https://github.com/crytic/medusa/blob/62b648c16053d87e3244dfd14a55371059be701c/chain/standard_cheat_code_contract.go#L449-L455

This can panic, because the input string can be less than 32-bytes. But it doesn't have to, even if it is. Our unit test tests this method by copying "medusa", a string less than 32-bytes.

Although you'd think this would trigger a panic, in Go you can successfully slice more elements from a slice than the length of the slice itself. Slicing is about capacity, not length in Go. If you think of a byte slice as a raw block of memory, where capacity is the size of the whole block, and length is what we're using of it. You can slice beyond length and get unused zero bytes reserved for that capacity. Note: Those bytes can be used if you append(b, byte(7)) for instance, but if you run out of capacity from appending too much, that would be when you get a new slice pointer to a new object (chunk of memory) with a higher capacity.

More importantly, the capacity of the slice would be non-deterministic unless it was allocated with an explicit capacity (e.g. specified with b := make([]byte, length, capacity). Our case wasn't explicitly allocated, but we'd usually just get a capacity>32 byte slice anyways, even though its length was less, and this would not panic. Rarely, you might've not. It seemed to have triggered on our Windows CI but not macOS/Linux.

TLDR: There was a b[:32] on a slice with len(b) < 32. It wouldn't always panic because slicing in Go does not consider length as much as capacity. Example: https://go.dev/play/p/g7I_jeNIE3W

The fix here is to simply not slice with a given length, and use copy(dst, src) regardless, as it's documentation states that it will already only copy the minimum amount of elements among the two slices.