Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/lib/state/LibInterpreterStateDataContract.sol
Original file line number Diff line number Diff line change
Expand Up @@ -141,4 +141,31 @@ library LibInterpreterStateDataContract {
);
}
}

/// @notice Returns the bytecode portion of a serialized `(constants, bytecode)`
/// blob as produced by `unsafeSerialize` (and so by `IParserV2.parse2`),
/// referenced in place without copying. The layout is
/// `[constants length][constants data][bytecode length][bytecode data]`, so
/// the bytecode begins after the constants block — the same skip
/// `unsafeDeserialize` performs. Returns empty bytes when `serialized` is too
/// short to hold both length words or declares a constants length that would
/// overrun it, so a caller introspecting untrusted input reads an empty
/// (zero source) bytecode rather than reading out of bounds.
/// @param serialized The serialized blob to read.
/// @return bc The embedded rain bytecode, referenced in place.
function bytecodeOf(bytes memory serialized) internal pure returns (bytes memory bc) {
if (serialized.length < 0x40) {
return "";
}
uint256 constantsLength;
assembly ("memory-safe") {
constantsLength := mload(add(serialized, 0x20))
}
if (constantsLength > (serialized.length - 0x40) / 0x20) {
return "";
}
assembly ("memory-safe") {
bc := add(add(serialized, 0x20), mul(0x20, add(constantsLength, 1)))
}
Comment on lines +156 to +169

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the target file
ls -la
fd -t f 'LibInterpreterStateDataContract\.sol' . || true

# Print the function and nearby code (bounded output)
FILE="src/lib/state/LibInterpreterStateDataContract.sol"
if [ -f "$FILE" ]; then
  echo "---- ${FILE} (around bytecodeOf) ----"
  rg -n "function bytecodeOf" "$FILE"
  # show ~80 lines around the match
  LINE=$(rg -n "function bytecodeOf" "$FILE" | head -n1 | cut -d: -f1)
  START=$((LINE-60))
  END=$((LINE+80))
  sed -n "${START},${END}p" "$FILE" | nl -ba | sed -n "1,200p"
fi

# Find any other definitions/helpers that might validate bytecode length
if [ -f "$FILE" ]; then
  echo "---- search for bytecodeLength / offsets in ${FILE} ----"
  rg -n "bytecodeLength|bytecodeOffset|bytecodeOf\(" "$FILE"
fi

# Search for usages of bytecodeOf and unsafeDeserialize
echo "---- Usage search ----"
rg -n "bytecodeOf\(" . || true
rg -n "unsafeDeserialize|unsafeSerialize" . || true

# Search tests for malformed serialized blobs / bytecode length overrun
echo "---- Test search ----"
fd -t f -e sol test . || true
rg -n "serialized.*bytecode|bytecode.*length|constantsLength.*overrun|bytecode.*overrun|bytecodeOf" test . || true

Repository: rainlanguage/rainlang

Length of output: 2349


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="src/lib/state/LibInterpreterStateDataContract.sol"
if [ ! -f "$FILE" ]; then
  echo "Missing $FILE"
  exit 1
fi

echo "---- bytecodeOf implementation ----"
LINE=$(rg -n "function bytecodeOf" "$FILE" | head -n1 | cut -d: -f1)
START=$((LINE-20))
END=$((LINE+60))
sed -n "${START},${END}p" "$FILE" | nl -ba -w1 -s' ' || sed -n "${START},${END}p" "$FILE"

echo "---- Search within file for bytecode length checks ----"
rg -n "bytecodeLength|bytecodeOffset|mload\\(add\\(serialized,|serialized\\.length|constantsLength" "$FILE"

echo "---- usages of bytecodeOf and unsafeDeserialize ----"
rg -n "bytecodeOf\\(" src test
rg -n "unsafeDeserialize|unsafeSerialize" src test

echo "---- look for tests covering malformed bytecode length ----"
rg -n "bytecode.*length|overrun|malformed|serialized.*bytecode|constantsLength.*overrun|bytecodeOf\\(" test || true

echo "---- bytecodeOf call sites for untrusted input assumptions ----"
rg -n "bytecodeOf\\(" -S src || true

Repository: rainlanguage/rainlang

Length of output: 8183


🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE="src/lib/state/LibInterpreterStateDataContract.sol"
TEST="test/src/lib/state/LibInterpreterStateDataContract.t.sol"

python3 - <<'PY'
import itertools,sys
def dump(path, start, end):
    print(f"\n---- {path} lines {start}-{end} ----")
    with open(path,'r',encoding='utf-8') as f:
        for i,line in enumerate(f, start=1):
            if start <= i <= end:
                print(f"{i:4d} {line.rstrip()}")
dump("src/lib/state/LibInterpreterStateDataContract.sol", 1, 220)
dump("src/lib/state/LibInterpreterStateDataContract.sol", 220, 420)
PY

echo "---- bytecodeOf: search for bytecodeLength guard in production ----"
rg -n "bytecodeLength|mload\\(.*bytecode|bytecode.*overrun|bytecodeOf\\(" "$FILE"

echo "---- tests around bytecodeOf malformed inputs ----"
python3 - <<'PY'
path="test/src/lib/state/LibInterpreterStateDataContract.t.sol"
start=280
end=420
print(f"\n---- {path} lines {start}-{end} ----")
with open(path,'r',encoding='utf-8') as f:
    for i,line in enumerate(f, start=1):
        if start <= i <= end:
            print(f"{i:4d} {line.rstrip()}")
PY

echo "---- look for any malformed serialized crafting for bytecode length ----"
rg -n "bytecode.*length|bytecodeLength|malformed|overrun|serialized\\[|mstore|assembly" "$TEST" || true

Repository: rainlanguage/rainlang

Length of output: 14401


Add a bytecode-length bounds check in LibInterpreterStateDataContract.bytecodeOf (lines 156-169).

bytecodeOf only bounds-checks constantsLength to ensure the bytecode length word is in-range, but it never validates the embedded bytecodeLength against the remaining bytes in serialized. A malformed blob can therefore return an in-place bytes slice with an oversized length, causing out-of-bounds reads/leaky ABI encoding when callers use the result.

Guard bytecodeLength before returning bc by loading it from serialized at bytecodeOffset = 0x40 + constantsLength * 0x20 and returning "" unless bytecodeLength <= serialized.length - bytecodeOffset.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/state/LibInterpreterStateDataContract.sol` around lines 156 - 169, In
LibInterpreterStateDataContract.bytecodeOf, after computing constantsLength (and
before returning bc) load the embedded bytecodeLength from serialized at
bytecodeOffset = 0x40 + constantsLength * 0x20 (use assembly "memory-safe"
mload(add(serialized, bytecodeOffset))) and validate that bytecodeLength <=
serialized.length - bytecodeOffset; if the check fails return "" instead of bc.
Ensure you compute bytecodeOffset the same way as when deriving bc and reference
symbols constantsLength, bytecodeOffset, bytecodeLength, and bc in the new
validation.

}
}
41 changes: 41 additions & 0 deletions test/src/lib/state/LibInterpreterStateDataContract.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -311,4 +311,45 @@ contract LibInterpreterStateDataContractTest is Test {
assertEq(lengths[0], stackAllocation0);
assertEq(lengths[1], stackAllocation1);
}

/// `bytecode` returns exactly the body `unsafeDeserialize` references, for
/// fuzzed constants — the constants-skip in both must agree.
function testBytecodeMatchesDeserialize(bytes32[] memory constants) external view {
bytes memory expected = buildTwoSourceBytecode(3, 5);
bytes memory serialized = serialize(expected, constants);

bytes memory got = LibInterpreterStateDataContract.bytecodeOf(serialized);
assertEq(got.length, expected.length);
assertEq(keccak256(got), keccak256(expected));

InterpreterState memory state = iExtern.deserialize(
serialized, 0, FullyQualifiedNamespace.wrap(0), IInterpreterStoreV3(address(0)), new bytes32[][](0), ""
);
assertEq(keccak256(got), keccak256(state.bytecode));
}

/// `bytecode` round-trips a single-source body behind fuzzed constants.
function testBytecodeSingleSource(bytes32[] memory constants) external pure {
bytes memory expected = buildSingleSourceBytecode(1, 2, 0, 1);
bytes memory serialized = serialize(expected, constants);
bytes memory got = LibInterpreterStateDataContract.bytecodeOf(serialized);
assertEq(got.length, expected.length);
assertEq(keccak256(got), keccak256(expected));
}

/// A blob too short to hold both length words yields empty bytecode.
function testBytecodeTooShortIsEmpty(bytes memory serialized) external view {
vm.assume(serialized.length < 0x40);
assertEq(LibInterpreterStateDataContract.bytecodeOf(serialized).length, 0);
}

/// A declared constants length that overruns the blob yields empty bytecode.
function testBytecodeOverrunConstantsIsEmpty(uint256 constantsLength) external view {
constantsLength = bound(constantsLength, 1, type(uint256).max);
bytes memory serialized = new bytes(0x40);
assembly ("memory-safe") {
mstore(add(serialized, 0x20), constantsLength)
}
assertEq(LibInterpreterStateDataContract.bytecodeOf(serialized).length, 0);
}
Comment on lines +315 to +354

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use complete NatSpec tags on the newly added tests to satisfy audit policy.

These new test doc comments are descriptive, but not fully tagged NatSpec entries as required by your test audit guideline.

As per coding guidelines, "test/**/*.{sol,rs}: All test files must meet audit requirements ... including ... complete NatSpec".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/src/lib/state/LibInterpreterStateDataContract.t.sol` around lines 315 -
354, Add complete NatSpec tags for each new test function so they meet audit
policy: update the doc comments for testBytecodeMatchesDeserialize,
testBytecodeSingleSource, testBytecodeTooShortIsEmpty, and
testBytecodeOverrunConstantsIsEmpty to use full NatSpec tags (e.g., `@notice` and
`@dev`, and `@param/`@return where applicable) instead of plain comments; ensure
each tag describes purpose, behavior, and any parameters (for functions with
arguments like bytes32[] memory constants, bytes memory serialized, and uint256
constantsLength) and that the comments precede the corresponding function
declarations.

Source: Coding guidelines

Comment on lines +340 to +354

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add a malformed bytecode-length overrun test case.

Current edge tests cover short blobs and constants overruns, but not a blob where constants are in-bounds and the embedded bytecode length overruns the payload.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/src/lib/state/LibInterpreterStateDataContract.t.sol` around lines 340 -
354, Add a new test that constructs a serialized blob where the constants length
word is in-bounds but the embedded bytecode length field claims more bytes than
remain (so bytecode overruns the payload) and assert
LibInterpreterStateDataContract.bytecodeOf(serialized).length == 0; build the
blob similar to existing tests in LibInterpreterStateDataContract.t.sol:
allocate a 0x40+N bytes buffer, write a valid small constantsLength at offset
0x20, then write a bytecodeLength at the offset where bytecode length is stored
that is larger than the actual remaining bytes, and call the same assertion as
in testBytecodeTooShortIsEmpty/testBytecodeOverrunConstantsIsEmpty to ensure an
overrun-by-bytecode case is covered.

}
Loading