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.

Sign up for GitHub

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

Remediations re 7739 update #216

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c4f9b0f
fix iterate over validators
Nov 8, 2024
cdd4d5f
opt 7739 check
Nov 8, 2024
792a770
revert not return
Nov 8, 2024
f40701f
add proper 7739 detection
Nov 11, 2024
3c6af1b
inline doc move
Nov 11, 2024
0fd6769
fix
Nov 11, 2024
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
20 changes: 11 additions & 9 deletions contracts/Nexus.sol
View file
Open in desktop
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,9 @@
/// @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

if (checkERC7739Support(hash, signature)) return SUPPORTS_ERC7739;
}
// 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 @@ -335,14 +338,13 @@
/// and return 0xffffffff as a result.
function checkERC7739Support(bytes32 hash, bytes calldata signature) public view virtual returns (bool) {
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;
}
// Forces the compiler to optimize for smaller bytecode size.
if (uint256(hash) == (~signature.length / 0xffff) * 0x7739) {
Copy link
Contributor

Choose a reason for hiding this comment

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

curious what does ~ do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is bitwise negation. so it makes 0xfff..fff out of 0.

SentinelListLib.SentinelList storage validators = _getAccountStorage().validators;
address next = validators.entries[SENTINEL];
while (next != ZERO_ADDRESS && next != SENTINEL) {

Check warning on line 345 in contracts/Nexus.sol

View check run for this annotation

Codecov / codecov/patch

contracts/Nexus.sol#L343-L345 

Added lines #L343 - L345 were not covered by tests
if (IValidator(next).isValidSignatureWithSender(msg.sender, hash, signature) == SUPPORTS_ERC7739) return true;
next = validators.getNext(next);

Check warning on line 347 in contracts/Nexus.sol

View check run for this annotation

Codecov / codecov/patch

contracts/Nexus.sol#L347 

Added line #L347 was not covered by tests
}
}
}
Expand Down
1 change: 0 additions & 1 deletion contracts/mocks/MockValidator.sol
View file
Open in desktop
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
13 changes: 2 additions & 11 deletions contracts/modules/validators/K1Validator.sol
View file
Open in desktop
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
36 changes: 25 additions & 11 deletions test/foundry/unit/concrete/erc1271/TestERC1271Account_IsValidSignature.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,59 +19,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 +104,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,7 +120,7 @@ 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));
}

Expand Down
4 changes: 2 additions & 2 deletions test/foundry/unit/concrete/modules/TestK1Validator.t.sol
View file
Open in desktop
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
Loading
Loading