Summary
The validateUserOp() method should validate user's signature and nonce.
However, it doesn't do that.
Vulnerability Details
validateUserOp() method inherits documentations from IAccount.sol#L7-L33:
* Validate user's signature and nonce
* the entryPoint will make the call to the recipient only if this validation call returns successfully.
* signature failure should be reported by returning SIG_VALIDATION_FAILED (1).
* This allows making a "simulation call" without a valid signature
* Other failures (e.g. nonce mismatch, or invalid signature format) should still revert to signal failure.
*
* @dev Must validate caller is the entryPoint.
* Must validate the signature and nonce
* @param userOp - The operation that is about to be executed.
* @param userOpHash - Hash of the user's request data. can be used as the basis for signature.
* @param missingAccountFunds - Missing funds on the account's deposit in the entrypoint.
* This is the minimum amount to transfer to the sender(entryPoint) to be
* able to make the call. The excess is left as a deposit in the entrypoint
* for future calls. Can be withdrawn anytime using "entryPoint.withdrawTo()".
* In case there is a paymaster in the request (or the current deposit is high
* enough), this value will be zero.
* @return validationData - Packaged ValidationData structure. use `_packValidationData` and
* `_unpackValidationData` to encode and decode.
* <20-byte> sigAuthorizer - 0 for valid signature, 1 to mark signature failure,
* otherwise, an address of an "authorizer" contract.
* <6-byte> validUntil - Last timestamp this operation is valid. 0 for "indefinite"
* <6-byte> validAfter - First timestamp this operation is valid
* If an account doesn't use time-range, it is enough to
* return SIG_VALIDATION_FAILED value (1) for signature failure.
* Note that the validation code cannot use block.timestamp (or block.number) directly.
*/
function validateUserOp(
It must validate signature and nonce. However, it doesn't do that. Let's analyze the validateUserOp() method:
function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash, uint256 missingAccountFunds)
external
virtual
override
requireFromEntryPoint
returns (uint256 validationData)
{
validationData = _validateSignature(userOp, userOpHash);
_validateNonce(userOp.nonce);
_payPrefund(missingAccountFunds);
}
To validate signature, it calls the _validateSignature() method:
function _validateSignature(PackedUserOperation calldata userOp, bytes32 userOpHash)
internal
pure
returns (uint256 validationData)
{
bytes32 hash = MessageHashUtils.toEthSignedMessageHash(userOpHash);
ECDSA.recover(hash, userOp.signature);
return SIG_VALIDATION_SUCCESS;
}
It uses ECDSA.recover(hash, userOp.signature) to recover the address, but it doesn't use the recovered address to make comparison with userOp.sender value. In this way, _validateSignature() method returns always SIG_VALIDATION_SUCCESS
To valid nonce, it calls the _validateNonce() method
which is empty and thus doesn't check nonce:
function _validateNonce(uint256 nonce) internal view virtual {}
Impact
The behavior of validateUserOp() method is different from the one described in IAccount.sol#L7-L33. This method never returns SIG_VALIDATION_FAILED and never reverts due to nonce mismatch, or invalid signature format, for example.
In this way, it cannot be used to make "simulation call", because it doesn't return the wanted values.
Tools Used
Visual inspection
Recommendations
Let's change the _validateSignature method in this way:
function _validateSignature(PackedUserOperation calldata userOp, bytes32 userOpHash)
internal
pure
returns (uint256 validationData)
{
bytes32 hash = MessageHashUtils.toEthSignedMessageHash(userOpHash);
- ECDSA.recover(hash, userOp.signature);
- return SIG_VALIDATION_SUCCESS;
+ if (userOp.sender == ECDSA.recover(hash, userOp.signature) return SIG_VALIDATION_SUCCESS;
+ return SIG_VALIDATION_FAILED
}
and write the _validateNonce() method.
