Conversation
`taint_copy_destination` was the only writer of `fmp_could_be_unbounded` for copy shaped writes, and neither the `MCopy` nor the `ExternalCall` arm of the heap analysis reached it. An `mcopy` destination or a call return range covering `[0x40, 0x60)` therefore tainted the word, which only disables the native mode read, while the `FMP < heap_size` range proof stayed active and truncated the clobbered `mload(0x40)`. The coverage check now lives in `flag_write_covering_fmp`, which the copy opcodes, `mcopy` and the external call return range all consult. A write of statically zero length never covers the slot, so solc's `call(.., pos, 0)` followed by `returndatacopy` keeps the optimization. Regression fixture `CopyFmpBug.yul` covers an `mcopy` onto a calldata supplied destination and a `staticcall` returning into `0x40`. Fixes paritytech/bugbounty_reports#216 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
elle-j
left a comment
elle-j
left a comment
There was a problem hiding this comment.
I'd suggest getting #612 merged before this one (including the comments there), and then merging main into this PR before addressing the comments below. 612 already fixes some of these issues (I specify which one in the comments).
| let len = self.extract_static_offset(length); | ||
|
|
||
| let covers_fmp = match (destination_start, len) { | ||
| (_, Some(0)) => false, |
There was a problem hiding this comment.
This is a regression for some copies, but #612 will fix this once it's been merged.
It regresses copies to a dynamic destination where the length is a loop counter starting at 0. (_, Some(0)) would treat the copy as zero-length (skipping the (None, _) dynamic destination check that set fmp_could_be_unbounded before this PR). For instance the mload(0x40) below is now truncated:
for { let n := 0 } lt(n, 0x40) { n := add(n, 0x20) } {
calldatacopy(calldataload(32), 0, n)
}
mstore(0, mload(0x40))
return(0, 32)Similarly here where the destination is e.g. 0x40:
mstore(0x80, calldataload(0))
for { let n := 0 } lt(n, 0x40) { n := add(n, 0x20) } {
mcopy(0x40, 0x80, n)
}
mstore(0, mload(0x40))
return(0, 32)| (_, Some(0)) => false, | ||
| (Some(address), Some(size)) => address < 0x60 && address.saturating_add(size) > 0x40, | ||
| (Some(address), None) => (0x40..0x60).contains(&address), | ||
| (None, _) => !self.is_free_pointer_relative(destination.id.0), |
There was a problem hiding this comment.
Seeing some code size regression (at -Oz) (e.g. FiatTokenV2 and FiatTokenV2_1 +23.6%, FiatTokenV2_2 +21%, XENCrypto +9%, UniswapV2Pair +9%).
That seem to be attributed to the range proof being dropped for the entire contract, triggered by mcopy in the emitted abi_encode_* functions:
abi_encode_string(value, pos)usesmcopy(add(pos, 0x20), ...)abi_encode_bytes(headStart, value0)usesmcopy(add(headStart, 64), ...)
So they'll reach this dynamic destination ((None, _)) arm. Then is_free_pointer_relative() doesn't see those destinations as free pointer relative (since they're derived from the encoder's own parameters, pos or headStart) so fmp_could_be_unbounded will be set to true.
Having it set to true shouldn't be necessary since solc only calls those functions with positions at or above the address the fmp holds (so at least 0x80).
One way to fix it may be to treat a parameter as free pointer relative when every call site passes an argument that is provably free pointer relatve, but haven't looked further into that.
| let covers_fmp = match (destination_start, len) { | ||
| (_, Some(0)) => false, | ||
| (Some(address), Some(size)) => address < 0x60 && address.saturating_add(size) > 0x40, | ||
| (Some(address), None) => (0x40..0x60).contains(&address), |
There was a problem hiding this comment.
(Some(address), None): With a dynamic length, a static start anywhere below 0x60 can reach the fmp word, but this arm only flags a start inside [0x40, 0x60). E.g. mcopy(0, source, length) overwrites the word if length > 0x40.
(Turns out this is also reachable from e.g. staticcalls with staic bounds after our fuzzy dedup pass merges out-of-line helpers that only differ in the return length like staticcall(..., 0, 0x60)) and staticcall(..., 0, 0x20), into one with the length as a parameter).
For mcopy and call returns, could this arm just check address < 0x60? (Since the arm already implies a dynamic length.)
I don't think #612 fixes this.
| ### Fixed | ||
| - `--newyork`: A bug in dead call value analysis. [#589](https://github.com/paritytech/revive/pull/589) | ||
| - `--newyork`: Object code that fell off the end was not terminated with the implicit EVM return, so LLVM folded away the runtime dispatch and a call would have run the constructor. [#598](https://github.com/paritytech/revive/pull/598) | ||
| - `--newyork`: an `mcopy` destination or an external call return range covering the free memory pointer word did not disable the `FMP < heap_size` range proof, which truncated the clobbered `mload(0x40)`. |
There was a problem hiding this comment.
| - `--newyork`: an `mcopy` destination or an external call return range covering the free memory pointer word did not disable the `FMP < heap_size` range proof, which truncated the clobbered `mload(0x40)`. | |
| - `--newyork`: an `mcopy` destination or an external call return range that overwrote the free memory pointer word did not disable the `FMP < heap_size` range proof, which truncated the clobbered `mload(0x40)`. [#613](https://github.com/paritytech/revive/pull/613) |
This branch has not been deployed
2 participants
An
mcopydestination or external call return range covering[0x40, 0x60)tainted the word but never setfmp_could_be_unbounded, so the range proof truncated the clobberedmload(0x40).