Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remediations re 7739 update #216

Merged
merged 6 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions contracts/Nexus.sol
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
using ModeLib for ExecutionMode;
using ExecLib for bytes;
using NonceLib for uint256;
using SentinelListLib for SentinelListLib.SentinelList;

/// @dev The timelock period for emergency hook uninstallation.
uint256 internal constant _EMERGENCY_TIMELOCK = 1 days;
Expand Down Expand Up @@ -226,7 +227,12 @@
/// @dev Delegates the validation to a validator module specified within the signature data.
function isValidSignature(bytes32 hash, bytes calldata signature) external view virtual override returns (bytes4) {
// Handle potential ERC7739 support detection request
if (checkERC7739Support(hash, signature)) return SUPPORTS_ERC7739;
if (signature.length == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if signature length is 0 then check? is this correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's in 7739 spec

// Forces the compiler to optimize for smaller bytecode size.
if (uint256(hash) == (~signature.length / 0xffff) * 0x7739) {
return checkERC7739Support(hash, signature);

Check warning on line 233 in contracts/Nexus.sol

View check run for this annotation

Codecov / codecov/patch

contracts/Nexus.sol#L233

Added line #L233 was not covered by tests
}
}
// else proceed with normal signature verification

// First 20 bytes of data will be validator address and rest of the bytes is complete signature.
Expand Down Expand Up @@ -333,20 +339,20 @@
/// If no validator supports ERC-7739, this function returns false
/// thus the account will proceed with normal signature verification
/// and return 0xffffffff as a result.
function checkERC7739Support(bytes32 hash, bytes calldata signature) public view virtual returns (bool) {
function checkERC7739Support(bytes32 hash, bytes calldata signature) public view virtual returns (bytes4) {
bytes4 result;

Check warning on line 343 in contracts/Nexus.sol

View check run for this annotation

Codecov / codecov/patch

contracts/Nexus.sol#L342-L343

Added lines #L342 - L343 were not covered by tests
unchecked {
if (signature.length == uint256(0)) {
// Forces the compiler to optimize for smaller bytecode size.
if (uint256(hash) == (~signature.length / 0xffff) * 0x7739) {
SentinelListLib.SentinelList storage validators = _getAccountStorage().validators;
address next = validators.entries[SENTINEL];
while (next != ZERO_ADDRESS && next != SENTINEL) {
if (IValidator(next).isValidSignatureWithSender(msg.sender, hash, signature) == SUPPORTS_ERC7739) return true;
}
SentinelListLib.SentinelList storage validators = _getAccountStorage().validators;
address next = validators.entries[SENTINEL];
while (next != ZERO_ADDRESS && next != SENTINEL) {

Check warning on line 347 in contracts/Nexus.sol

View check run for this annotation

Codecov / codecov/patch

contracts/Nexus.sol#L345-L347

Added lines #L345 - L347 were not covered by tests
bytes4 support = IValidator(next).isValidSignatureWithSender(msg.sender, hash, signature);
if (bytes2(support) == bytes2(SUPPORTS_ERC7739) && support > result) {
result = support;
}
next = validators.getNext(next);
}
}
return false;
return result == bytes4(0) ? bytes4(0xffffffff) : result;
}

/// @dev Ensures that only authorized callers can upgrade the smart contract implementation.
Expand Down
1 change: 0 additions & 1 deletion contracts/mocks/MockValidator.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ contract MockValidator is ERC7739Validator {
bytes calldata signature
) external view virtual returns (bytes4 sigValidationResult) {
// can put additional checks based on sender here

return _erc1271IsValidSignatureWithSender(sender, hash, _erc1271UnwrapSignature(signature));
}

Expand Down
95 changes: 95 additions & 0 deletions contracts/mocks/MockValidator_7739v2.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.27;

import { IModule } from "../interfaces/modules/IModule.sol";
import { IModuleManager } from "../interfaces/base/IModuleManager.sol";
import { VALIDATION_SUCCESS, VALIDATION_FAILED, MODULE_TYPE_VALIDATOR, ERC1271_MAGICVALUE, ERC1271_INVALID } from "../types/Constants.sol";
import { PackedUserOperation } from "account-abstraction/interfaces/PackedUserOperation.sol";
import { ECDSA } from "solady/utils/ECDSA.sol";
import { SignatureCheckerLib } from "solady/utils/SignatureCheckerLib.sol";
import { MessageHashUtils } from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol";
import { ERC7739Validator } from "erc7739Validator/ERC7739Validator.sol";

contract MockValidator_7739v2 is ERC7739Validator {
mapping(address => address) public smartAccountOwners;

function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash) external view returns (uint256 validation) {
address owner = smartAccountOwners[msg.sender];
return _validateSignatureForOwner(owner, userOpHash, userOp.signature) ? VALIDATION_SUCCESS : VALIDATION_FAILED;
}

function isValidSignatureWithSender(
address sender,
bytes32 hash,
bytes calldata signature
) external view virtual returns (bytes4) {
return 0x77390002;
}

// ISessionValidator interface for smart session
function validateSignatureWithData(bytes32 hash, bytes calldata sig, bytes calldata data) external view returns (bool validSig) {
address owner = address(bytes20(data[0:20]));
return _validateSignatureForOwner(owner, hash, sig);
}

function _validateSignatureForOwner(address owner, bytes32 hash, bytes calldata signature) internal view returns (bool) {
if (SignatureCheckerLib.isValidSignatureNowCalldata(owner, hash, signature)) {
return true;
}
if (SignatureCheckerLib.isValidSignatureNowCalldata(owner, MessageHashUtils.toEthSignedMessageHash(hash), signature)) {
return true;
}
return false;
}

/// @dev Returns whether the `hash` and `signature` are valid.
/// Obtains the authorized signer's credentials and calls some
/// module's specific internal function to validate the signature
/// against credentials.
function _erc1271IsValidSignatureNowCalldata(bytes32 hash, bytes calldata signature) internal view override returns (bool) {
// obtain credentials
address owner = smartAccountOwners[msg.sender];

// call custom internal function to validate the signature against credentials
return _validateSignatureForOwner(owner, hash, signature);
}

/// @dev Returns whether the `sender` is considered safe, such
/// that we don't need to use the nested EIP-712 workflow.
/// See: https://mirror.xyz/curiousapple.eth/pFqAdW2LiJ-6S4sg_u1z08k4vK6BCJ33LcyXpnNb8yU
// The canonical `MulticallerWithSigner` at 0x000000000000D9ECebf3C23529de49815Dac1c4c
// is known to include the account in the hash to be signed.
// msg.sender = Smart Account
// sender = 1271 og request sender
function _erc1271CallerIsSafe(address sender) internal view virtual override returns (bool) {
return (sender == 0x000000000000D9ECebf3C23529de49815Dac1c4c || // MulticallerWithSigner
sender == msg.sender);
}

function onInstall(bytes calldata data) external {
require(IModuleManager(msg.sender).isModuleInstalled(MODULE_TYPE_VALIDATOR, address(this), ""), "Validator is still installed");
smartAccountOwners[msg.sender] = address(bytes20(data));
}

function onUninstall(bytes calldata data) external {
data;
require(!IModuleManager(msg.sender).isModuleInstalled(MODULE_TYPE_VALIDATOR, address(this), ""), "Validator is still installed");
delete smartAccountOwners[msg.sender];
}

function isModuleType(uint256 moduleTypeId) external pure returns (bool) {
return moduleTypeId == MODULE_TYPE_VALIDATOR;
}

function isOwner(address account, address owner) external view returns (bool) {
return smartAccountOwners[account] == owner;
}

function isInitialized(address) external pure returns (bool) {
return false;
}

function getOwner(address account) external view returns (address) {
return smartAccountOwners[account];
}
}
13 changes: 2 additions & 11 deletions contracts/modules/validators/K1Validator.sol
Original file line number Diff line number Diff line change
Expand Up @@ -158,23 +158,14 @@ contract K1Validator is IValidator, ERC7739Validator {
* @return sigValidationResult the result of the signature validation, which can be:
* - EIP1271_SUCCESS if the signature is valid
* - EIP1271_FAILED if the signature is invalid
* - 0x7739000X if this is the ERC-7739 support detection request.
* Where X is the version of the ERC-7739 support.
*/
function isValidSignatureWithSender(
address sender,
bytes32 hash,
bytes calldata signature
) external view virtual override returns (bytes4) {
// sig malleability prevention
// only needed here as 4337 flow has nonce
bytes32 s;
assembly {
// same as `s := mload(add(signature, 0x40))` but for calldata
s := calldataload(add(signature.offset, 0x20))
}
if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
return 0xffffffff;
}

return _erc1271IsValidSignatureWithSender(sender, hash, _erc1271UnwrapSignature(signature));
}

Expand Down
3 changes: 2 additions & 1 deletion contracts/types/Constants.sol
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,5 @@ bytes32 constant MODULE_ENABLE_MODE_TYPE_HASH = keccak256(bytes(MODULE_ENABLE_MO
bytes1 constant MODE_VALIDATION = 0x00;
bytes1 constant MODE_MODULE_ENABLE = 0x01;

bytes4 constant SUPPORTS_ERC7739 = 0x77390001;
bytes4 constant SUPPORTS_ERC7739 = 0x77390000;
bytes4 constant SUPPORTS_ERC7739_V1 = 0x77390001;
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pragma solidity ^0.8.27;

import "../../../utils/Imports.sol";
import "../../../utils/NexusTest_Base.t.sol";
import "contracts/mocks/MockValidator_7739v2.sol";

/// @title TestERC1271Account_IsValidSignature
/// @notice This contract tests the ERC1271 signature validation functionality.
Expand All @@ -19,59 +20,73 @@ contract TestERC1271Account_IsValidSignature is NexusTest_Base {
uint256 missingAccountFunds;
}

K1Validator private validator;

bytes32 internal constant APP_DOMAIN_SEPARATOR = 0xa1a044077d7677adbbfa892ded5390979b33993e0e2a457e3f974bbcda53821b;

/// @notice Initializes the testing environment.
function setUp() public {
init();
validator = new K1Validator();
bytes memory callData =
abi.encodeWithSelector(IModuleManager.installModule.selector, MODULE_TYPE_VALIDATOR, address(validator), abi.encodePacked(ALICE_ADDRESS));
// Create an execution array with the installation call data
Execution[] memory execution = new Execution[](1);
execution[0] = Execution(address(ALICE_ACCOUNT), 0, callData);

// Build a packed user operation for the installation
PackedUserOperation[] memory userOps = buildPackedUserOperation(ALICE, ALICE_ACCOUNT, EXECTYPE_DEFAULT, execution, address(VALIDATOR_MODULE), 0);

// Execute the user operation to install the modules
ENTRYPOINT.handleOps(userOps, payable(BOB.addr));
}

/// @notice Tests the validation of a personal signature using the mock validator.
function test_isValidSignature_PersonalSign_MockValidator_Success() public {
function test_isValidSignature_PersonalSign_K1Validator_Success() public {
TestTemps memory t;
t.contents = keccak256("123");
bytes32 hashToSign = toERC1271HashPersonalSign(t.contents, address(ALICE_ACCOUNT));
(t.v, t.r, t.s) = vm.sign(ALICE.privateKey, hashToSign);
bytes memory signature = abi.encodePacked(t.r, t.s, t.v);
assertEq(ALICE_ACCOUNT.isValidSignature(t.contents, abi.encodePacked(address(VALIDATOR_MODULE), signature)), bytes4(0x1626ba7e));
assertEq(ALICE_ACCOUNT.isValidSignature(t.contents, abi.encodePacked(address(validator), signature)), bytes4(0x1626ba7e));

unchecked {
uint256 vs = uint256(t.s) | (uint256(t.v - 27) << 255);
signature = abi.encodePacked(t.r, vs);
assertEq(ALICE_ACCOUNT.isValidSignature(t.contents, abi.encodePacked(address(VALIDATOR_MODULE), signature)), bytes4(0x1626ba7e));
assertEq(ALICE_ACCOUNT.isValidSignature(t.contents, abi.encodePacked(address(validator), signature)), bytes4(0xffffffff));
}
}

/// @notice Tests the validation of an EIP-712 signature using the mock validator.
function test_isValidSignature_EIP712Sign_MockValidator_Success() public {
function test_isValidSignature_EIP712Sign_K1Validator_Success() public {
TestTemps memory t;
t.contents = keccak256("0x1234");
bytes32 dataToSign = toERC1271Hash(t.contents, address(ALICE_ACCOUNT));
(t.v, t.r, t.s) = vm.sign(ALICE.privateKey, dataToSign);
bytes memory contentsType = "Contents(bytes32 stuff)";
bytes memory signature = abi.encodePacked(t.r, t.s, t.v, APP_DOMAIN_SEPARATOR, t.contents, contentsType, uint16(contentsType.length));
if (random() % 4 == 0) signature = erc6492Wrap(signature);
bytes4 ret = ALICE_ACCOUNT.isValidSignature(toContentsHash(t.contents), abi.encodePacked(address(VALIDATOR_MODULE), signature));
bytes4 ret = ALICE_ACCOUNT.isValidSignature(toContentsHash(t.contents), abi.encodePacked(address(validator), signature));
assertEq(ret, bytes4(0x1626ba7e));

unchecked {
uint256 vs = uint256(t.s) | (uint256(t.v - 27) << 255);
signature = abi.encodePacked(t.r, vs, APP_DOMAIN_SEPARATOR, t.contents, contentsType, uint16(contentsType.length));
assertEq(
ALICE_ACCOUNT.isValidSignature(toContentsHash(t.contents), abi.encodePacked(address(VALIDATOR_MODULE), signature)),
bytes4(0x1626ba7e)
ALICE_ACCOUNT.isValidSignature(toContentsHash(t.contents), abi.encodePacked(address(validator), signature)),
bytes4(0xffffffff)
);
}
}

/// @notice Tests the failure of an EIP-712 signature validation due to a wrong signer.
function test_isValidSignature_EIP712Sign_MockValidator_Wrong1271Signer_Fail() public view {
function test_isValidSignature_EIP712Sign_K1Validator_Wrong1271Signer_Fail() public view {
TestTemps memory t;
t.contents = keccak256("123");
(t.v, t.r, t.s) = vm.sign(BOB.privateKey, toERC1271Hash(t.contents, address(ALICE_ACCOUNT)));
bytes memory contentsType = "Contents(bytes32 stuff)";
bytes memory signature = abi.encodePacked(t.r, t.s, t.v, APP_DOMAIN_SEPARATOR, t.contents, contentsType, uint16(contentsType.length));
bytes4 ret = ALICE_ACCOUNT.isValidSignature(toContentsHash(t.contents), abi.encodePacked(address(VALIDATOR_MODULE), signature));
bytes4 ret = ALICE_ACCOUNT.isValidSignature(toContentsHash(t.contents), abi.encodePacked(address(validator), signature));
assertEq(ret, bytes4(0xFFFFFFFF));
}

Expand All @@ -90,7 +105,7 @@ contract TestERC1271Account_IsValidSignature is NexusTest_Base {
// Wrap the original signature using the ERC6492 format
bytes memory wrappedSignature = erc6492Wrap(signature);

bytes4 ret = ALICE_ACCOUNT.isValidSignature(toContentsHash(t.contents), abi.encodePacked(address(VALIDATOR_MODULE), wrappedSignature));
bytes4 ret = ALICE_ACCOUNT.isValidSignature(toContentsHash(t.contents), abi.encodePacked(address(validator), wrappedSignature));
assertEq(ret, bytes4(0x1626ba7e));
}

Expand All @@ -106,18 +121,22 @@ contract TestERC1271Account_IsValidSignature is NexusTest_Base {
bytes memory contentsType = "Contents(bytes32 stuff)";
bytes memory signature = abi.encodePacked(t.r, t.s, t.v, APP_DOMAIN_SEPARATOR, t.contents, contentsType, uint16(contentsType.length));

bytes4 ret = ALICE_ACCOUNT.isValidSignature(toContentsHash(t.contents), abi.encodePacked(address(VALIDATOR_MODULE), signature));
bytes4 ret = ALICE_ACCOUNT.isValidSignature(toContentsHash(t.contents), abi.encodePacked(address(validator), signature));
assertEq(ret, bytes4(0x1626ba7e));
}

/// @notice Tests the ERC7739 support detection request.
function test_ERC7739SupportDetectionRequest() public {
MockValidator_7739v2 validator_7739v2 = new MockValidator_7739v2();
vm.prank(address(ENTRYPOINT));
ALICE_ACCOUNT.installModule(MODULE_TYPE_VALIDATOR, address(validator_7739v2), abi.encodePacked(ALICE_ADDRESS));
assertTrue(ALICE_ACCOUNT.isModuleInstalled(MODULE_TYPE_VALIDATOR, address(validator_7739v2), ""));
assertEq(
ALICE_ACCOUNT.isValidSignature(
0x7739773977397739773977397739773977397739773977397739773977397739,
""
),
SUPPORTS_ERC7739
bytes4(0x77390002) // SUPPORTS_ERC7739_V2
);
}

Expand Down
4 changes: 2 additions & 2 deletions test/foundry/unit/concrete/modules/TestK1Validator.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,8 @@ contract TestK1Validator is NexusTest_Base {
// invert signature
signedMessage = abi.encodePacked(r, s1, v == 27 ? 28 : v);
vm.prank(address(BOB_ACCOUNT));
result = validator.isValidSignatureWithSender(address(this), originalHash, signedMessage);
assertEq(result, ERC1271_INVALID, "Signature with invalid 's' value should be rejected");
vm.expectRevert(bytes4(keccak256("InvalidSignature()")));
validator.isValidSignatureWithSender(address(this), originalHash, signedMessage);
}

function test_IsValidSignatureWithSender_SafeCaller_Success() public {
Expand Down
2 changes: 1 addition & 1 deletion test/foundry/utils/TestHelper.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { NexusBootstrap, BootstrapConfig } from "../../../contracts/utils/NexusB
import { BiconomyMetaFactory } from "../../../contracts/factory/BiconomyMetaFactory.sol";
import { NexusAccountFactory } from "../../../contracts/factory/NexusAccountFactory.sol";
import { BootstrapLib } from "../../../contracts/lib/BootstrapLib.sol";
import { MODE_VALIDATION } from "../../../contracts/types/Constants.sol";
import { MODE_VALIDATION, SUPPORTS_ERC7739_V1 } from "../../../contracts/types/Constants.sol";
import { MockRegistry } from "../../../contracts/mocks/MockRegistry.sol";
import { HelperConfig } from "../../../scripts/foundry/HelperConfig.s.sol";

Expand Down
Loading
Loading