🔒 M-05 - Revert on Validator Not Installed in validateUserOp() #121

Aboudjem · 2024-07-31T11:36:39Z

M-05. `Nexus.validateUserOp()` violates the EIP-4337 specification

Issue: validateUserOp() does not revert when the validator is not installed.
Affected Functions: validateUserOp.
Fix: Replace return VALIDATION_FAILED with require(_isValidatorInstalled(validator), InvalidModule(validator)).

…functions in TestK1Validator

…n.tree

…h erc4337

…t instead of returning invalid

…of returning invalid

codecov · 2024-07-31T11:46:03Z

Codecov Report

Attention: Patch coverage is 70.31250% with 19 lines in your changes missing coverage. Please review.

Project coverage is 75.61%. Comparing base (b1f51b2) to head (cd4d8de).
Report is 24 commits behind head on remediations/cantina-spearbit.

Files	Patch %	Lines
contracts/base/ModuleManager.sol	70.00%	6 Missing ⚠️
contracts/modules/validators/K1Validator.sol	58.33%	5 Missing ⚠️
contracts/Nexus.sol	78.94%	4 Missing ⚠️
contracts/base/BaseAccount.sol	0.00%	2 Missing ⚠️
contracts/factory/NexusAccountFactory.sol	50.00%	1 Missing ⚠️
contracts/factory/RegistryFactory.sol	50.00%	1 Missing ⚠️

Additional details and impacted files

@@                        Coverage Diff                        @@
##           remediations/cantina-spearbit     #121      +/-   ##
=================================================================
- Coverage                          75.67%   75.61%   -0.07%     
=================================================================
  Files                                 13       13              
  Lines                                666      693      +27     
  Branches                             154      141      -13     
=================================================================
+ Hits                                 504      524      +20     
- Misses                               162      169       +7

Files	Coverage Δ
contracts/base/Storage.sol	`66.66% <ø> (ø)`
contracts/factory/BiconomyMetaFactory.sol	`93.33% <100.00%> (ø)`
contracts/factory/K1ValidatorFactory.sol	`100.00% <100.00%> (ø)`
contracts/utils/RegistryBootstrap.sol	`100.00% <100.00%> (ø)`
contracts/factory/NexusAccountFactory.sol	`93.75% <50.00%> (-6.25%)`	⬇️
contracts/factory/RegistryFactory.sol	`82.69% <50.00%> (-1.31%)`	⬇️
contracts/base/BaseAccount.sol	`61.11% <0.00%> (+1.65%)`	⬆️
contracts/Nexus.sol	`64.36% <78.94%> (+1.51%)`	⬆️
contracts/modules/validators/K1Validator.sol	`83.33% <58.33%> (-7.15%)`	⬇️
contracts/base/ModuleManager.sol	`82.58% <70.00%> (-1.35%)`	⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bd4851...cd4d8de. Read the comment docs.

contracts/Nexus.sol

livingrockrises

left comment. almost approved..

livingrockrises · 2024-08-10T14:33:00Z

test/foundry/unit/fuzz/TestFuzz_ValidateUserOp.tree

-│ └── it should fail the validation
-└── when validating user operation with an invalid user address
- └── it should fail the validation
+├── when validating user operations with a valid signature and sufficient funds


not sure if this is making any sense..

├── when validating user operations with an invalid signature
│ └── it should fail validation with InvalidModule error
├── when validating user operations with an invalid nonce
│ └── it should fail validation with InvalidModule error
├── when validating user operations with an invalid nonce and valid signature
│ └── it should fail validation with InvalidModule error
└── when validating user operations with an invalid user address
└── it should fail validation with InvalidModule error

test/foundry/unit/concrete/erc4337account/TestERC4337Account_ValidateUserOp.t.sol

VGabriel45

@Aboudjem have a look over the comments, if there is not way to improve the error handling at least change revert reason name as Chirag suggested

MrToph

this is the relevant part of the EIP

If the account does not support signature aggregation, it MUST validate that the signature is a valid signature of the userOpHash, and SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch. Any other error MUST revert.

MrToph · 2024-08-19T04:28:06Z

contracts/Nexus.sol

- // Check if validator is not enabled. If not, return VALIDATION_FAILED.
- if (!_isValidatorInstalled(validator)) return VALIDATION_FAILED;
+ // Check if validator is not enabled. If not, revert.
+ require(_isValidatorInstalled(validator), InvalidModule(validator));


this should then also apply to both if and else branches in https://github.com/bcnmy/nexus/pull/139/files as both branches there return this constant

livingrockrises · 2024-08-19T12:32:59Z

M-05. Nexus.validateUserOp() violates the EIP-4337 specification

Issue: validateUserOp() does not revert when the validator is not installed.

Affected Functions: validateUserOp.

Fix: Replace return VALIDATION_FAILED with require(_isValidatorInstalled(validator), InvalidModule(validator)).

so there are two functions,
_hasValidators() [ purpose of this is to check if no validator is installed ]

and
_isValidatorInstalled [ purpose of this is to check if there is specific validator installed or not ]

implementation recap

        return _getAccountStorage().validators.contains(validator);
    }

function _hasValidators() internal view returns (bool) {
       return _getAccountStorage().validators.getNext(address(0x01)) != address(0x01) && 
       _getAccountStorage().validators.getNext(address(0x01)) != address(0x00);
   }
   ```
   (above is slightly changed after @MrToph 's comment )
   

error corresponding to _isValidatorInstalled is > ValidatorNotInstalled(validator)
error corresponding to _hasValidators is > NoValidatorInstalled()

it's only logical right?

now there is one place
function _uninstallValidator(address validator, bytes calldata data) internal virtual {
where _hasValidators is used and the error is CanNotRemoveLastValidator() as it is more logical than NoValidatorInstalled right?

there is one more error defined InvalidModule(_module) 
this was used wrongly at other above places.
but I have still kept it to be used at below places,

/// @notice Ensures the message sender is a registered executor module.
modifier onlyExecutorModule() virtual {
require(_getAccountStorage().executors.contains(msg.sender), InvalidModule(msg.sender));
_;
}

/// @notice Ensures the given validator is a registered validator module.
modifier onlyValidatorModule(address validator) {
    require(_getAccountStorage().validators.contains(validator), InvalidModule(validator));
    _;
}
```

some foundry tests may fail now as I haven't built and checked. but I have made code changes and made sure 1. it compiles 2. hardhat tests run.
once we agree on above we can easily fix foundry tests if we have to.

wdyt?
@filmakarov @VGabriel45 @Aboudjem @MrToph

livingrockrises · 2024-08-19T12:34:58Z

I also need final review on stage of validateUserOp, as it may have bricked enable mode logic so please @filmakarov

function validateUserOp(
        PackedUserOperation calldata op,
        bytes32 userOpHash,
        uint256 missingAccountFunds
    )
        external
        virtual
        payPrefund(missingAccountFunds)
        onlyEntryPoint
        returns (uint256 validationData)
    {
        address validator = op.nonce.getValidator();
        if (op.nonce.isModuleEnableMode()) {
            PackedUserOperation memory userOp = op;
            userOp.signature = _enableMode(userOpHash, op.signature);
            require(_isValidatorInstalled(validator), ValidatorNotInstalled(validator));
            validationData = IValidator(validator).validateUserOp(userOp, userOpHash);
        } else {
            require(_isValidatorInstalled(validator), ValidatorNotInstalled(validator));
            validationData = IValidator(validator).validateUserOp(op, userOpHash);
        }    
    }

there is one doubt now, we are never returning VALIDATION_FAILED in this function.
It should be returned only when signature validation fails, rest all cases should revert.
I believe code is fine cause it is validation's module's job and validationData would have been returned accordingly.

wdyt? @MrToph

filmakarov · 2024-08-20T18:08:24Z

I agree that it should revert always but when signature is actually invalid.
Validators would pack 1 to validation data when it is the case.

As for enable mode, it shouldn't be bricked, as it is always an already installed validator now in the nonce, so I'm fine with merging it as soon as corresponding tests pass

livingrockrises · 2024-08-20T18:27:06Z

tests are passing now. please check latest commit
cd4d8de

github-actions · 2024-08-20T18:28:33Z

🤖 Slither Analysis Report 🔎

Slither report

# Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary

constable-states (2 results) (Optimization)

constable-states

Impact: Optimization
🔴 Confidence: High

ID-0
RegistryAdapter.registry should be constant

base/RegistryAdapter.sol#L10

ID-1
RegistryFactory.threshold should be constant

factory/RegistryFactory.sol#L39

_This comment was automatically generated by the GitHub Actions workflow._

VGabriel45 · 2024-08-21T06:29:21Z

require(_isValidatorInstalled(validator), ValidatorNotInstalled(validator));

What's the point of this require statement for enableMode?
require(_isValidatorInstalled(validator), ValidatorNotInstalled(validator));
at this stage the validator will be installed, otherwise if something went wrong it would have reverted in _enableMode
@filmakarov

Aboudjem added 18 commits July 29, 2024 10:32

🔒️ h01 fix by adding msg.value

482e6cc

fix: handle inner call failure and msgvalue in factory contracts

c10eb71

refactor Bootstrap contract to improve code organization

c264761

feat: Add validation for 's' value in K1Validator

692a1db

feat: Add tests for isValidSignatureWithSender and transferOwnership …

1d4cbb8

…functions in TestK1Validator

feat: Add validation for 's' value in K1Validator

5a75c21

feat: Add validation for 's' value in K1Validator

60b93a9

feat: Add Storage contract and update BaseAccount to use it

ef5c7b6

add new modifier onlyAuthorized

303369f

feat: fix account creation test in TestK1ValidatorFactory_Deployments

6f94787

feat: Add _ENTRYPOINT address to Storage contract

5a9eacf

refactor: Update fallback function tests in TextNexus_FallbackFunctio…

6ac9161

…n.tree

feat: Improve validation for 's' value in K1Validator

3f48ffb

revert when invalid validation module on validateuserop to comply wit…

95a26dc

…h erc4337

refactor: Update TestERC4337Account_ValidateUserOp.t.sol to use rever…

ec8d6bb

…t instead of returning invalid

refactor: Update TestFuzz_ERC4337Account.t.sol to use revert instead …

12ea0f2

…of returning invalid

refactor: Update TestFuzz_ValidateUserOp.t.sol to use revert instead …

429f01f

…of returning invalid

format fix

684a39c

Aboudjem requested review from filmakarov, ankurdubey521, VGabriel45 and livingrockrises July 31, 2024 11:36

Aboudjem self-assigned this Jul 31, 2024

Aboudjem changed the title ~~Fix/security m 05~~ 🔒 M-05 - Revert on Validator Not Installed in validateUserOp() Jul 31, 2024

livingrockrises reviewed Aug 10, 2024

View reviewed changes

contracts/Nexus.sol Outdated Show resolved Hide resolved

livingrockrises reviewed Aug 10, 2024

View reviewed changes

VGabriel45 reviewed Aug 12, 2024

View reviewed changes

test/foundry/unit/concrete/erc4337account/TestERC4337Account_ValidateUserOp.t.sol Outdated Show resolved Hide resolved

VGabriel45 reviewed Aug 12, 2024

View reviewed changes

VGabriel45 approved these changes Aug 16, 2024

View reviewed changes

MrToph approved these changes Aug 19, 2024

View reviewed changes

MrToph reviewed Aug 19, 2024

View reviewed changes

livingrockrises mentioned this pull request Aug 19, 2024

Fix/cantina fixes #139

Merged

livingrockrises changed the base branch from fix/security-m03 to remediations/cantina-spearbit August 19, 2024 11:15

Merge branch 'remediations/cantina-spearbit' into fix/security-m-05

d7f9219

refactor

7f5050a

fix foundry tests

cd4d8de

livingrockrises merged commit 8199fab into remediations/cantina-spearbit Aug 20, 2024
7 of 8 checks passed

livingrockrises deleted the fix/security-m-05 branch August 20, 2024 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔒 M-05 - Revert on Validator Not Installed in validateUserOp() #121

🔒 M-05 - Revert on Validator Not Installed in validateUserOp() #121

Aboudjem commented Jul 31, 2024

codecov bot commented Jul 31, 2024 •

edited

Loading

livingrockrises left a comment

livingrockrises Aug 10, 2024

Aboudjem Aug 12, 2024

VGabriel45 left a comment

MrToph left a comment

MrToph Aug 19, 2024

livingrockrises commented Aug 19, 2024

M-05. `Nexus.validateUserOp()` violates the EIP-4337 specification

livingrockrises commented Aug 19, 2024

filmakarov commented Aug 20, 2024 •

edited

Loading

livingrockrises commented Aug 20, 2024

github-actions bot commented Aug 20, 2024

constable-states

VGabriel45 commented Aug 21, 2024 •

edited

Loading

🔒 M-05 - Revert on Validator Not Installed in validateUserOp() #121

🔒 M-05 - Revert on Validator Not Installed in validateUserOp() #121

Conversation

Aboudjem commented Jul 31, 2024

M-05. Nexus.validateUserOp() violates the EIP-4337 specification

codecov bot commented Jul 31, 2024 • edited Loading

Codecov Report

livingrockrises left a comment

Choose a reason for hiding this comment

livingrockrises Aug 10, 2024

Choose a reason for hiding this comment

Aboudjem Aug 12, 2024

Choose a reason for hiding this comment

VGabriel45 left a comment

Choose a reason for hiding this comment

MrToph left a comment

Choose a reason for hiding this comment

MrToph Aug 19, 2024

Choose a reason for hiding this comment

livingrockrises commented Aug 19, 2024

M-05. Nexus.validateUserOp() violates the EIP-4337 specification

livingrockrises commented Aug 19, 2024

filmakarov commented Aug 20, 2024 • edited Loading

livingrockrises commented Aug 20, 2024

github-actions bot commented Aug 20, 2024

🤖 Slither Analysis Report 🔎

Slither report

constable-states

VGabriel45 commented Aug 21, 2024 • edited Loading

M-05. `Nexus.validateUserOp()` violates the EIP-4337 specification

codecov bot commented Jul 31, 2024 •

edited

Loading

M-05. `Nexus.validateUserOp()` violates the EIP-4337 specification

filmakarov commented Aug 20, 2024 •

edited

Loading

VGabriel45 commented Aug 21, 2024 •

edited

Loading