Skip to content

Commit

Permalink
Merge pull request #216 from bcnmy/fix/cantina-notes-on-7739-update
Browse files Browse the repository at this point in the history
Remediations re 7739 update
  • Loading branch information
filmakarov authored Nov 11, 2024
2 parents d015436 + 0fd6769 commit 72499c9
Show file tree
Hide file tree
Showing 9 changed files with 319 additions and 215 deletions.
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 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
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 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
/// @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) {
// Forces the compiler to optimize for smaller bytecode size.
if (uint256(hash) == (~signature.length / 0xffff) * 0x7739) {
return checkERC7739Support(hash, signature);
}
}
// 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 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
/// 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;
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) {
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

0 comments on commit 72499c9

Please sign in to comment.