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

[H-1] `MondrianWallet::_ValidateSignature` has improper validation which may lead to complete loss of `owner` funds

codexbugmenot

Description

MondrianWallet::_ValidateSignature function should validate a signature and cross check it with MondrianWallet::owner but it isn't actually being checked in the function which may allow any Malicious Users transaction to be validated even if it is not signed by MondrianWallet::owner

Impact

Any Malicious User can create a PackedUserOperation with a target MondrianWallet address as sender and any valid signature , can get validated by MondrianWallet::_ValidateSignaturesince there is no proper validation in place i.e The Recovered Addresses is not validated if it belongs to MondrianWallet::owner this may lead to complete loss of owner funds if the malicious user crafts a calldata to drain the MondrianWallet

Proof of Concept

The statement ECDSA.recover recovers the address of the signer and returns it but it is nevercompared with the MondrianWallet::owner , any malicious user can create a valid signature and bypass the validation.

PoC
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;
}
​
Foundry Stacks Output

Look at the points marked with @> in the below output

  1. The wallet owner is 0x7c8999dC9a822c1f0Df42023113EDB4FDd543266

  2. The Signature is signed by 0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266's private key the first account in local nodes

  3. The Validation only checks if the signer is the one who signed the userOp hence it returns 0x000000000000000000000000f39fd6e51aad88f6f4ce6ab8827279cfffb92266 the recovered address

  4. It never cross checks if this address belongs to MondrianWallet::owner and concludes the validation as SIG_VALIDATION_SUCCESS by returning 0

[2376] MondrianWallet::owner() [staticcall]
@> │ └─ ← [Return] owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [65634] MockEntryPoint::handleOps([PackedUserOperation({ sender: 0x88F59F8826af5e695B13cA934d6c7999875A9EeA, nonce: 0, initCode: 0x, callData: 0xb61d27f600000000000000000000000029e3b139f4393adda86303fcdaa35f60bb7092bf00000000000000000000000000000000000000000000000029a2241af62c000000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000000, accountGasLimits: 0x0000000000000000000000001000000000000000000000000100000000000000, preVerificationGas: 100000 [1e5], gasFees: 0x0000000000000000000000000000000000000000000000000000000000000000, paymasterAndData: 0x, signature: 0x62b02a729594a2cc83545734606fde62cc0e9440b4715c985f74526f02903f534838d31cb247bd5199788d5ac110b58ea56a01ae97461e87c50d38ec7d54f6d81c })], user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ ├─ [4861] MondrianWallet::validateUserOp(PackedUserOperation({ sender: 0x88F59F8826af5e695B13cA934d6c7999875A9EeA, nonce: 0, initCode: 0x, callData: 0xb61d27f600000000000000000000000029e3b139f4393adda86303fcdaa35f60bb7092bf00000000000000000000000000000000000000000000000029a2241af62c000000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000000, accountGasLimits: 0x0000000000000000000000001000000000000000000000000100000000000000, preVerificationGas: 100000 [1e5], gasFees: 0x0000000000000000000000000000000000000000000000000000000000000000, paymasterAndData: 0x, signature: 0x62b02a729594a2cc83545734606fde62cc0e9440b4715c985f74526f02903f534838d31cb247bd5199788d5ac110b58ea56a01ae97461e87c50d38ec7d54f6d81c }), 0x7958e85bcc66a5f01d6249e6e3e2cb3209941d4f42ae447d2dd672872449bc99, 0)
│ │ ├─ [3000] PRECOMPILES::ecrecover(0xf9234f71220a5ba0c00ab807fdea6edb46113a16fc53d65cd4dc974234a86aaa, 28, 44637917207094263696392455209769621152118706081383040999288154546084963696467, 32666925575755548820907399020455965736064593565167791783803635721205471049432) [staticcall]
@> │ │ │ └─ ← [Return] 0x000000000000000000000000f39fd6e51aad88f6f4ce6ab8827279cfffb92266
@> │ │ └─ ← [Return] 0

Recommended Mitigation

Once the ECDSA.recover is done in MondrianWallet::_ValidateSignature and an address is returned ,it should be cross checked with MondrianWallet::owner to ensure if the owner is the actual signer of the signature provided

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

Lead Judging Commences

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

ECDSA.recover should check against sender

Support

FAQs

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