Category: Coding Mistakes
Add Length Validation for callData in validateSessionUserOp
Informational Severity
Informational Impact
N/A Likelihood
Description
In the validateSessionUserOp function, the length check for op.callData is incomplete, and there may be callData with illegal length.
function validateSessionUserOp(
UserOperation calldata _op,
bytes32 _userOpHash,
bytes calldata _sessionKeyData,
bytes calldata _sessionKeySignature
) external pure override returns (bool) {
...
// working with userOp.callData
// check if the call is to the allowed recepient and amount is not more than allowed
bytes calldata data;
{
uint256 offset = uint256(bytes32(_op.callData[4 + 64:4 + 96]));
uint256 length = uint256(
bytes32(_op.callData[4 + offset:4 + offset + 32])
);
//we expect data to be the `IERC20.transfer(address, uint256)` calldata
data = _op.callData[4 + offset + 32:4 + offset + 32 + length];
}
! if (address(bytes20(data[16:36])) != recipient) {
revert("ERC20SV Wrong Recipient");
}
! if (uint256(bytes32(data[36:68])) > maxAmount) {
revert("ERC20SV Max Amount Exceeded");
}
return
ECDSA.recover(
ECDSA.toEthSignedMessageHash(_userOpHash),
_sessionKeySignature
) == sessionKey;
}Impact
Data without length restrictions may lead to issues such as hash collisions. A hash collision may lead to the ability to arbitrarily forge user messages.
Recommendations
Use abi.decode to get the message length and add a maximum length check on op.callData.
Remediation
This issue has been acknowledged by Biconomy Labs, and a fix was implemented in commit 3bf128e9↗.