Possible issue in `ERC7579Utils.decodeBatch`

[Amxx]

Recently, v5.2-rc.1 included a fix to a known issue in ERC7579Utils.decodeBatch. The issue was identified as originating from missing checks that solidity automatically inserts when doing a decode operation. These checks were not present in v5.2-rc.0

The way these checks are currently implemented is as follows:

• when decoding bytes (in calldata) as an array, we check that the bytes is long enough so that all the "first-level elements" of the array fit inside the bytes. Because the array contains dynamic objects, the "first-level elements" are only pointers (offsets) to the actual elements.
• during the decoding, it is NOT checked that the the target of the pointers falls inside the calldata bytes object. It is only when dereferencing these "pointers" that solidity checks weither they are "valid".

However, solidity does that by checking that the dereferenced object is in the calldata as defined by calldatasize. This means that the pointers could target data that is outside thebytes object while still being in allocated calldata.

This could potentially cause issues, though it is not 100% clear how this could be exploited and what the consequences would be. Currently no POC of this bad behavior exists.

Signers of ERC-7579 batch operations (usually within the scope of of an ERC-4337 user operation) should verify that the calldata they sign is properly formed. A possible adversarial senario would exist if the signer is a session key with limited access to the account. This issue, if confirmed, could potentially cause the validation and execution phases to missmatch, allowing the session key to perform restricted operations.

[frangio]

though it is not 100% clear how this could be exploited and what the consequences would be

It is possible to create a user op where executionCalldata appears "valid" during validateUserOp but "invalid" during execute, so that decodeBatch only reverts in execute phase. This can happen because calldatasize is strictly larger during validateUserOp than it is during execute.

It seems that the extent of an exploit would be to cause the account to pay for gas for a user op that should've been rejected in an earlier phase.

Signers of ERC-7579 batch operations should verify that the calldata they sign is properly formed. A possible adversarial senario would exist if the signer is a session key with limited access to the account.

The question we should answer is whether the account should protect against malformed executionCalldata. If an account is exclusively controlled by a signer, that signer is able to always construct well-formed user ops, or to check well-formedness before signing one. But if the account has other authorization options, perhaps with more autonomy like session keys, then this may be a way to grief the account and make it pay for gas that was used for nothing -- although perhaps that could be considered a bug of the session key design.

[Amxx]

The issue described above requires that the calldata bytes that is decoded includes offset to elements that are outside the bytes while still being inside the calldatasize. This can only happen if the bytes object is NOT the last element of the calldata.

ERC-7579 includes two function that deal with executionCalldata that may be decoded:

function execute(bytes32 mode, bytes calldata executionCalldata) external;
function executeFromExecutor(bytes32 mode, bytes calldata executionCalldata) external returns (bytes[] memory returnData);

In both cases the executionCalldata is the last element. That means that any pointer that goes beyond it would also go beyond the calldatasize. That would cause the dereferencing (in solidity) to fail. This makes us believe that any malformed bytes passed to either of these functions would cause it to fail. Therefore, usage of decodeBatch in the context of an ERC-7579 execution is safe.

In the context of an ERC-4337 operation, we still have to consider the validation ot the user operation.

In the validation stage, the calldata will be part of the PackedUserOperation. Offsets in the decoded calldata could point to any (dynamic) part of the user operation beyound the calldata section itself. That includes paymasterAndData and signature. This is what we see in the v5.2-rc.0 bug, and is still possible in the validation phase. Said otherwise, mallformed calldata, if decoded during validation, could be interpreted in an unconventional way ... but as we saw before the following execution would revert.

Technically, a signer would be able to sign a mallformed calldata that would be missinterpreted during validation (if validation does the decoding... in most cases it wont), but that would then fail execution anyway. That would grief gas from the account/paymaster. This griefing is however not different from that caused by any user operation that passes validation and reverts during execution.

It could be possible to trigger the early failure during the validation phase. This would require traversing the structure in depth during the decoding, and checking all offsets are not spilling beyond the input bytes. However I believe we should not implement that:

• It would negativelly impact the gas costs of all decoding
• The most common usecase decodes only during ERC-7579 execution, which is not affected as discussed above
• Decoding the ERC-7579 batch during validation is a corner case. Most of the time it won't happen.
• When the validation does decode the batch, the execution is going to fail anyway.
• The gas grieffing that is possible in the corner case when validation does decode the batch would likelly be possible even if decodeBatch was "fixed".

Are there any other context where we expect all the calls to ERC7579Utils.decodeBatch to be be performed on buffers that are NOT the last element in calldata? An eligible context would NOT include an ERC-7579 execute/executeFromExecutor call.