NY: flag mcopy and call return ranges that clobber the free pointer

CHANGELOG.md

@@ -10,6 +10,7 @@ Supported `polkadot-sdk` rev: `2604.2.0`
 
 ### Fixed
 - `--newyork`: A bug in dead call value analysis. [#589](https://github.com/paritytech/revive/pull/589)
+- `--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)`.
 
 ## v1.4.0
 

crates/integration/contracts/CopyFmpBug.yul

@@ -0,0 +1,22 @@
+/// An `mcopy` onto a calldata supplied destination and a `staticcall` returning into `0x40`.
+object "CopyFmpBug" {
+  code { datacopy(0, dataoffset("CopyFmpBug_deployed"), datasize("CopyFmpBug_deployed")) return(0, datasize("CopyFmpBug_deployed")) }
+  object "CopyFmpBug_deployed" {
+    code {
+      switch calldataload(0)
+      case 1 {
+        mstore(0x80, calldataload(32))
+        mcopy(and(calldataload(64), 0xff), 0x80, 0x20)
+      }
+      case 2 {
+        if iszero(staticcall(gas(), address(), 0, 0, 0x40, 0x20)) { revert(0, 0) }
+      }
+      default {
+        mstore(0, not(0))
+        return(0, 32)
+      }
+      mstore(0, mload(0x40))
+      return(0, 32)
+    }
+  }
+}

crates/integration/contracts/FmpMcopyStraddle.sol

@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8;
+
+/// Reproducer from paritytech/bugbounty_reports#216.
+contract FmpMcopyStraddle {
+    function probe() external view returns (uint256 r) {
+        assembly {
+            mstore(0x80, calldataload(0))
+            mcopy(0x38, 0x80, 42)
+            r := mload(0x40)
+            mstore(0x40, 0x80)
+        }
+    }
+}

crates/integration/contracts/FmpStaticcallReturn.sol

@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8;
+
+/// Reproducer from paritytech/bugbounty_reports#216.
+contract FmpStaticcallReturn {
+    function probe() external view returns (uint256 r) {
+        assembly {
+            mstore(0x80, 0x100000000000000000000000000000000000000000000000007)
+            pop(staticcall(gas(), 4, 0x80, 0x20, 0x40, 0x20))
+            mstore(add(0x2000, calldatasize()), 0)
+            r := mload(0x40)
+            mstore(0x40, 0x80)
+        }
+    }
+}

crates/integration/src/tests.rs

@@ -4984,6 +4984,51 @@ fn calldatacopy_dynamic_dest_fmp_corruption() {
     run_differential(actions);
 }
 
+/// An `mcopy` or a call return range over the free memory pointer word must disable the range proof.
+#[test]
+fn copy_onto_fmp_word_disables_range_proof() {
+    let copied_word: U256 = (U256::from(1u64) << 200) | U256::from(0xabcdefu64);
+    let mut mcopy_case = U256::from(1).to_be_bytes::<32>().to_vec();
+    mcopy_case.extend_from_slice(&copied_word.to_be_bytes::<32>());
+    mcopy_case.extend_from_slice(&U256::from(0x40).to_be_bytes::<32>());
+    let staticcall_case = U256::from(2).to_be_bytes::<32>().to_vec();
+    for data in [mcopy_case, staticcall_case] {
+        let mut actions = instantiate_yul("contracts/CopyFmpBug.yul", "CopyFmpBug");
+        actions.push(Call {
+            origin: TestAddress::Alice,
+            dest: TestAddress::Instantiated(0),
+            value: 0,
+            gas_limit: Some(GAS_LIMIT),
+            storage_deposit_limit: None,
+            data,
+        });
+        run_differential(actions);
+    }
+}
+
+/// Reproducers from paritytech/bugbounty_reports#216.
+#[test]
+fn fmp_staticcall_return_and_mcopy_straddle() {
+    let selector = keccak256(b"probe()")[..4].to_vec();
+    let mut mcopy_calldata = selector.clone();
+    mcopy_calldata.extend_from_slice(&[0xff; 32]);
+    for (contract, data) in [
+        ("FmpStaticcallReturn", selector),
+        ("FmpMcopyStraddle", mcopy_calldata),
+    ] {
+        let mut actions = instantiate(&format!("contracts/{contract}.sol"), contract);
+        actions.push(Call {
+            origin: TestAddress::Alice,
+            dest: TestAddress::Instantiated(0),
+            value: 0,
+            gas_limit: Some(GAS_LIMIT),
+            storage_deposit_limit: None,
+            data,
+        });
+        run_differential(actions);
+    }
+}
+
 /// Regression (newyork dead-store elimination): a store read back by an
 /// intervening unaligned *overlapping* load must not be eliminated as dead.
 /// `mem_opt` marked a pending store read only on an exact-offset load, so

crates/newyork/src/heap_opt.rs

@@ -363,6 +363,7 @@ impl HeapAnalysis {
                 let destination_start = self.extract_static_offset(destination);
                 let source_start = self.extract_static_offset(source);
                 let len = self.extract_static_offset(length);
+                self.flag_write_covering_fmp(destination, length);
                 self.taint_range(destination_start, len);
                 self.taint_range(source_start, len);
             }
@@ -376,6 +377,7 @@ impl HeapAnalysis {
             } => {
                 self.mark_escaping_range(args_offset, args_length);
                 self.note_fmp_coverage(args_offset, args_length);
+                self.flag_write_covering_fmp(ret_offset, ret_length);
                 self.mark_escaping_and_tainted_range(ret_offset, ret_length);
                 self.note_fmp_coverage(ret_offset, ret_length);
             }
@@ -589,6 +591,24 @@ impl HeapAnalysis {
         }
     }
 
+    /// Flags the free memory pointer as possibly unbounded when a raw write of `length`
+    /// bytes to `destination` can cover `[0x40, 0x60)`. A zero length never covers it,
+    /// and neither does a dynamic destination that is free pointer relative.
+    fn flag_write_covering_fmp(&mut self, destination: &Value, length: &Value) {
+        let destination_start = self.extract_static_offset(destination);
+        let len = self.extract_static_offset(length);
+
+        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),
+            (None, _) => !self.is_free_pointer_relative(destination.id.0),
+        };
+        if covers_fmp {
+            self.fmp_could_be_unbounded = true;
+        }
+    }
+
     /// Taints the destination of a copy opcode (`calldatacopy`, `codecopy`,
     /// `returndatacopy`, …), which writes big-endian bytes that a later native
     /// (little-endian) `mload` must not byte-reverse.
@@ -600,39 +620,11 @@ impl HeapAnalysis {
     /// native mode for the entire contract (e.g. every ABI-decode `calldatacopy`),
     /// a large code-size regression for no soundness gain over the existing
     /// dynamic-offset guards.
-    /// Records the memory tainted by a copy (`calldatacopy`/`codecopy`/`mcopy`/…) with
-    /// destination `destination` and length `length`, and flags the free-memory-pointer slot as
-    /// possibly unbounded when the copy can clobber it.
-    ///
-    /// A copy that can overwrite the FMP slot `[0x40, 0x60)` replaces the free-memory
-    /// pointer with arbitrary, possibly out-of-range bytes. Downstream codegen that assumes
-    /// `FMP < heap_size` (the narrow `mload(0x40)` read and its range proof) would then
-    /// mis-read the corrupted value, so such a copy sets `fmp_could_be_unbounded` — exactly
-    /// as an untrusted `mstore(0x40, ...)` does. Tainting word 0x40 alone only disables the
-    /// *native-mode* FMP read; the FMP *range proof* in `to_llvm` is gated on
-    /// `fmp_could_be_unbounded`, so that flag must be set too or the proof silently
-    /// truncates the clobbered value.
-    ///
-    /// `covers_fmp` is kept deliberately narrow to avoid a code-size regression: only a
-    /// static destination+length that provably overlap the slot, or a static destination
-    /// *inside* the FMP word with a dynamic length (whose first byte(s) land in the slot),
-    /// flag unboundedness. A fully-dynamic destination, or a static destination *outside*
-    /// the word (proxy `calldatacopy(0, 0, size)`, OZ's FMP-relative ABI-decode copies to
-    /// `mload(0x40) >= 0x80`), is left to `has_dynamic_accesses` / the native-mode guards.
     fn taint_copy_destination(&mut self, destination: &Value, length: &Value) {
         let destination_start = self.extract_static_offset(destination);
         let len = self.extract_static_offset(length);
 
-        let covers_fmp = match (destination_start, len) {
-            (Some(address), Some(size)) => {
-                size > 0 && address < 0x60 && address.saturating_add(size) > 0x40
-            }
-            (Some(address), None) => (0x40..0x60).contains(&address),
-            (None, _) => !self.is_free_pointer_relative(destination.id.0),
-        };
-        if covers_fmp {
-            self.fmp_could_be_unbounded = true;
-        }
+        self.flag_write_covering_fmp(destination, length);
 
         match (destination_start, len) {
             (Some(address), Some(size)) => {
@@ -2004,6 +1996,76 @@ mod tests {
         );
     }
 
+    fn object_with_mcopy(setup: Vec<Statement>, length: u64) -> Object {
+        use crate::ir::ValueId;
+        let mut statements = setup;
+        statements.push(literal(11, 0x80));
+        statements.push(literal(12, length));
+        statements.push(Statement::MCopy {
+            destination: Value::int(ValueId(10)),
+            source: Value::int(ValueId(11)),
+            length: Value::int(ValueId(12)),
+        });
+        object_with_code(statements, vec![])
+    }
+
+    fn object_with_external_call(setup: Vec<Statement>, return_length: u64) -> Object {
+        use crate::ir::{CallKind, ValueId};
+        let mut statements = setup;
+        statements.push(literal(11, return_length));
+        statements.push(literal(12, 0));
+        statements.push(Statement::ExternalCall {
+            kind: CallKind::StaticCall,
+            gas: Value::int(ValueId(12)),
+            address: Value::int(ValueId(12)),
+            value: None,
+            args_offset: Value::int(ValueId(12)),
+            args_length: Value::int(ValueId(12)),
+            ret_offset: Value::int(ValueId(10)),
+            ret_length: Value::int(ValueId(11)),
+            result: ValueId(13),
+        });
+        object_with_code(statements, vec![])
+    }
+
+    #[test]
+    fn mcopy_onto_fmp_word_flags_unbounded() {
+        let results = object_with_mcopy(vec![literal(10, 0x40)], 0x20).analyze_heap();
+        assert!(results.fmp_could_be_unbounded());
+    }
+
+    #[test]
+    fn mcopy_dynamic_destination_flags_unbounded() {
+        use crate::ir::ValueId;
+        let setup = vec![Statement::Let {
+            bindings: vec![ValueId(10)],
+            value: Expression::CallDataLoad {
+                offset: Value::int(ValueId(0)),
+            },
+        }];
+        let results = object_with_mcopy(setup, 0x20).analyze_heap();
+        assert!(results.fmp_could_be_unbounded());
+    }
+
+    #[test]
+    fn external_call_return_onto_fmp_word_flags_unbounded() {
+        let results = object_with_external_call(vec![literal(10, 0x40)], 0x20).analyze_heap();
+        assert!(results.fmp_could_be_unbounded());
+    }
+
+    #[test]
+    fn external_call_zero_length_return_stays_bounded() {
+        use crate::ir::ValueId;
+        let setup = vec![Statement::Let {
+            bindings: vec![ValueId(10)],
+            value: Expression::CallDataLoad {
+                offset: Value::int(ValueId(0)),
+            },
+        }];
+        let results = object_with_external_call(setup, 0).analyze_heap();
+        assert!(!results.fmp_could_be_unbounded());
+    }
+
     #[test]
     fn test_offset_info_from_literal() {
         let analysis = HeapAnalysis::new();