Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add checks to ERC7579Utils.decodeBatch #5353

Merged
merged 28 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from 9 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 contracts/account/utils/draft-ERC4337Utils.sol
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,33 @@ library ERC4337Utils {
return result;
}

/// @dev Variant of {hash} that supports user operations in memory.
function hashMemory(
PackedUserOperation memory self,
address entrypoint,
uint256 chainid
) internal pure returns (bytes32) {
bytes32 result = keccak256(
abi.encode(
keccak256(
abi.encode(
self.sender,
self.nonce,
keccak256(self.initCode),
keccak256(self.callData),
self.accountGasLimits,
self.preVerificationGas,
self.gasFees,
keccak256(self.paymasterAndData)
)
),
entrypoint,
chainid
)
);
return result;
}

/// @dev Returns `factory` from the {PackedUserOperation}, or address(0) if the initCode is empty or not properly formatted.
function factory(PackedUserOperation calldata self) internal pure returns (address) {
return self.initCode.length < 20 ? address(0) : address(bytes20(self.initCode[0:20]));
Expand Down
35 changes: 30 additions & 5 deletions contracts/account/utils/draft-ERC7579Utils.sol
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ library ERC7579Utils {
/// @dev The module type is not supported.
error ERC7579UnsupportedModuleType(uint256 moduleTypeId);

/// @dev Input calldata not properly formatted and possibly malicious.
error ERC7579DecodingError();

/// @dev Executes a single call.
function execSingle(
ExecType execType,
Expand Down Expand Up @@ -169,12 +172,34 @@ library ERC7579Utils {
}

/// @dev Decodes a batch of executions. See {encodeBatch}.
///
/// Note: This function runs some checks and will throw a {ERC7579DecodingError} if the input is not properly formatted.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
function decodeBatch(bytes calldata executionCalldata) internal pure returns (Execution[] calldata executionBatch) {
frangio marked this conversation as resolved.
Show resolved Hide resolved
assembly ("memory-safe") {
let ptr := add(executionCalldata.offset, calldataload(executionCalldata.offset))
// Extract the ERC7579 Executions
executionBatch.offset := add(ptr, 32)
executionBatch.length := calldataload(ptr)
unchecked {
uint256 bufferLength = executionCalldata.length;

// Check executionCalldata is not empty
if (bufferLength < 32) revert ERC7579DecodingError();

// Get the offset of the array, and check its valid
Amxx marked this conversation as resolved.
Show resolved Hide resolved
uint256 offset = uint256(bytes32(executionCalldata[0:32]));
ernestognw marked this conversation as resolved.
Show resolved Hide resolved

// Array length is between offset and offset + 32. We check that this is in the buffer
Amxx marked this conversation as resolved.
Show resolved Hide resolved
// Since we know executionCalldata is at least 32, we can subtract with no overflow risk.
if (offset > bufferLength - 32) revert ERC7579DecodingError();

// Get the array length
Amxx marked this conversation as resolved.
Show resolved Hide resolved
uint256 arrayLength = uint256(bytes32(executionCalldata[offset:offset + 32]));
if (arrayLength > type(uint64).max) revert ERC7579DecodingError();

// Get the array as a bytes slice, and check its is long enough
Amxx marked this conversation as resolved.
Show resolved Hide resolved
bytes calldata executionArray = executionCalldata[offset + 32:];
if (executionArray.length < arrayLength * 96) revert ERC7579DecodingError();
ernestognw marked this conversation as resolved.
Show resolved Hide resolved

Choose a reason for hiding this comment

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

I think the check for the struct's static-sized elements should be performed by solidity lazily, as you access each Execution struct. To make this match the loose ABI-decoder exactly, I think at this stage the only necessary check would be:

if (executionArray.length < arrayLength * 32)

So, essentially just checking that the struct pointers would still fit in the available space. To illustrate, here is a non-standard, but valid, encoding of 3 Executions that would fail this check, but succeed in abi.decode(...) or in the built-in ABI decoder at the top of a function:

0x
0000000000000000000000000000000000000000000000000000000000000020    // Offset of `Execution[]`
0000000000000000000000000000000000000000000000000000000000000003    // Length of `Execution[]`
0000000000000000000000000000000000000000000000000000000000000060    // Offset of `executions[0]`
0000000000000000000000000000000000000000000000000000000000000060    // Offset of `executions[1]` (same as `executions[0]`)
0000000000000000000000000000000000000000000000000000000000000060    // Offset of `executions[2]` (same as `executions[0]`)
000000000000000000000000dcaccbf48225b2b2af81351fe52fbd0edf02adff 	// `executions[0].to`
0000000000000000000000000000000000000000000000000000000000000001	// `executions[0].value`
0000000000000000000000000000000000000000000000000000000000000060	// Offset of `executions[0].data`
0000000000000000000000000000000000000000000000000000000000000000	// Length of `executions[0].data`

It has all 3 of the offsets point to the same location in calldata, making the total size 7 words, with is less than the expected 9 words.

Either way, it's fine if you keep it as-is, just wanted to flag that this check is ever so slightly too strict.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@frangio, since you looked at the encoding a bit more than I did, do you confirm that the array doesn't contain the "head" of the Execution but instead it contains offsets (pointers) to these heads?

If that is the case, we should indeed use 32 instead of 96

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 6aaa56a

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed with the ABI spec that arrayLength * 32 is the correct value.


assembly ("memory-safe") {
executionBatch.offset := executionArray.offset
executionBatch.length := arrayLength
}
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
Amxx marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
1 change: 1 addition & 0 deletions foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ out = 'out'
libs = ['node_modules', 'lib']
test = 'test'
cache_path = 'cache_forge'
fs_permissions = [{ access = "read", path = "./test/bin" }]

[fuzz]
runs = 5000
Expand Down
Loading
Loading