Skip to content

Commit

Permalink
Merge pull request #215 from bcnmy/fix/optimize-sig-malleability-prot…
Browse files Browse the repository at this point in the history 
…ection

Sig malleability protection for erc1271 only
  • Loading branch information
@livingrockrises
livingrockrises authored Nov 2, 2024
2 parents 69f437b + bbe68ea commit d015436
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 30 deletions.
34 changes: 22 additions & 12 deletions contracts/modules/validators/K1Validator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ contract K1Validator is IValidator, ERC7739Validator {
_safeSenders.remove(msg.sender, sender);
}

/// @notice Checks if a sender is in the _safeSenders list for the smart account
function isSafeSender(address sender, address smartAccount) external view returns (bool) {
return _safeSenders.contains(smartAccount, sender);
}

/**
* Check if the module is initialized
* @param smartAccount The smart account to check
Expand Down Expand Up @@ -141,11 +146,15 @@ contract K1Validator is IValidator, ERC7739Validator {

/**
* Validates an ERC-1271 signature
* @dev implements signature malleability prevention
* see: https://eips.ethereum.org/EIPS/eip-1271#reference-implementation
* Please note, that this prevention does not protect against replay attacks in general
* So the protocol using ERC-1271 should make sure hash is replay-safe.
*
* @param sender The sender of the ERC-1271 call to the account
* @param hash The hash of the message
* @param signature The signature of the message
*
*
* @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
Expand All @@ -154,7 +163,18 @@ contract K1Validator is IValidator, ERC7739Validator {
address sender,
bytes32 hash,
bytes calldata signature
) external view virtual override returns (bytes4 sigValidationResult) {
) 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 Expand Up @@ -231,16 +251,6 @@ contract K1Validator is IValidator, ERC7739Validator {
/// @param hash The hash of the data to validate
/// @param signature The signature data
function _validateSignatureForOwner(address owner, bytes32 hash, bytes calldata signature) internal view returns (bool) {
// Check if the 's' value is valid
bytes32 s;
assembly {
// same as `s := mload(add(signature, 0x40))` but for calldata
s := calldataload(add(signature.offset, 0x20))
}
if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
return false;
}

// verify signer
// owner can not be zero address in this contract
if (_recoverSigner(hash, signature) == owner) return true;
Expand Down
87 changes: 70 additions & 17 deletions test/foundry/unit/concrete/modules/TestK1Validator.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,21 +232,35 @@ contract TestK1Validator is NexusTest_Base {
assertEq(res, VALIDATION_SUCCESS, "Valid signature should be accepted");
}

/// @notice Tests that a signature with an invalid 's' value is rejected
function test_ValidateUserOp_InvalidSValue() public {
bytes32 originalHash = keccak256(abi.encodePacked("invalid message"));
(uint8 v, bytes32 r, bytes32 s) = vm.sign(BOB.privateKey, originalHash);
/// @notice Tests signature malleability is prevented by nonce in the 4337 flow
function test_ValidateUserOp_Inverted_S_Value_Fails_because_of_nonce() public {
Counter counter = new Counter();
//build and sign a userOp
Execution[] memory execution = new Execution[](1);
execution[0] = Execution(address(counter), 0, abi.encodeWithSelector(Counter.incrementNumber.selector));
PackedUserOperation[] memory userOps = buildPackedUserOperation(BOB, BOB_ACCOUNT, EXECTYPE_DEFAULT, execution, address(VALIDATOR_MODULE), 0);
ENTRYPOINT.handleOps(userOps, payable(BOB.addr));

// Ensure 's' is in the upper range (invalid)
if (uint256(s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
s = bytes32(0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1); // Set an invalid 's' value
// parse the userOp.signature via assembly
bytes32 r;
bytes32 s;
uint8 v;
assembly {
r := mload(add(userOps, 0x20))
s := mload(add(userOps, 0x40))
v := byte(0, mload(add(userOps, 0x60)))
}

userOp.signature = abi.encodePacked(r, s, v);

uint256 res = validator.validateUserOp(userOp, originalHash);
// invert signature
bytes32 s1;
if (uint256(s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
s1 = bytes32(115792089237316195423570985008687907852837564279074904382605163141518161494337 - uint256(s));
}
userOps[0].signature = abi.encodePacked(r, s1, v == 27 ? 28 : v);

assertEq(res, VALIDATION_FAILED, "Signature with invalid 's' value should be rejected");
bytes memory revertReason = abi.encodeWithSelector(FailedOp.selector, 0, "AA25 invalid account nonce");
vm.expectRevert(revertReason);
ENTRYPOINT.handleOps(userOps, payable(BOB.addr));
}

/// @notice Tests that a valid signature with a valid 's' value is accepted for isValidSignatureWithSender
Expand All @@ -272,20 +286,29 @@ contract TestK1Validator is NexusTest_Base {
}

/// @notice Tests that a signature with an invalid 's' value is rejected for isValidSignatureWithSender
function test_IsValidSignatureWithSender_InvalidSValue() public {
function test_IsValidSignatureWithSender_Inverted_S_Value_Fails() public {
// allow vanilla 1271 flow
vm.prank(address(BOB_ACCOUNT));
validator.addSafeSender(address(this));

bytes32 originalHash = keccak256(abi.encodePacked("invalid message"));
(uint8 v, bytes32 r, bytes32 s) = vm.sign(BOB.privateKey, originalHash);
bytes32 s1;

// Ensure 's' is in the upper range (invalid)
if (uint256(s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
s = bytes32(0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1); // Set an invalid 's' value
s1 = bytes32(115792089237316195423570985008687907852837564279074904382605163141518161494337 - uint256(s));
}

// assert original signature is valid
bytes memory signedMessage = abi.encodePacked(r, s, v);
bytes memory completeSignature = abi.encodePacked(address(validator), signedMessage);

bytes4 result = BOB_ACCOUNT.isValidSignature(originalHash, completeSignature);
vm.prank(address(BOB_ACCOUNT));
bytes4 result = validator.isValidSignatureWithSender(address(this), originalHash, signedMessage);
assertEq(result, ERC1271_MAGICVALUE, "Valid signature should be accepted");

// 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");
}

Expand Down Expand Up @@ -322,6 +345,36 @@ contract TestK1Validator is NexusTest_Base {
assertEq(mockSafe1271Caller.balanceOf(address(BOB_ACCOUNT)), 1);
}

/// @notice Tests the addSafeSender function to add a safe sender to the safe senders list
function test_addSafeSender_Success() public {
prank(address(BOB_ACCOUNT));
validator.addSafeSender(address(mockSafe1271Caller));
assertTrue(validator.isSafeSender(address(mockSafe1271Caller), address(BOB_ACCOUNT)), "MockSafe1271Caller should be in the safe senders list");
}

/// @notice Tests the removeSafeSender function to remove a safe sender from the safe senders list
function test_removeSafeSender_Success() public {
prank(address(BOB_ACCOUNT));
validator.removeSafeSender(address(mockSafe1271Caller));
assertFalse(validator.isSafeSender(address(mockSafe1271Caller), address(BOB_ACCOUNT)), "MockSafe1271Caller should be removed from the safe senders list");
}

/// @notice Tests the fillSafeSenders function to fill the safe senders list
function test_fillSafeSenders_Success() public {
prank(address(0x03));
validator.onInstall(abi.encodePacked(address(0xdecaf0), address(0x01), address(0x02)));
assertTrue(validator.isSafeSender(address(0x01), address(0x03)));
assertTrue(validator.isSafeSender(address(0x02), address(0x03)));
}

/// @notice Tests the isSafeSender function to check if a sender is in the safe senders list
function test_isSafeSender_Success() public {
assertFalse(validator.isSafeSender(address(0x01), address(0x03)));
prank(address(0x03));
validator.addSafeSender(address(0x01));
assertTrue(validator.isSafeSender(address(0x01), address(0x03)));
}

/// @notice Generates an ERC-1271 hash for personal sign
/// @param childHash The child hash
/// @return The ERC-1271 hash for personal sign
Expand Down
11 changes: 10 additions & 1 deletion test/foundry/unit/concrete/modules/TestK1Validator.tree
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ TestK1Validator
├── when testing the validateUserOp function
│ ├── it should succeed with a valid signature (toEthSignedMessageHash)
│ └── it should fail with an invalid signature
│ └── signature malleability should be rejected because of nonce
├── when testing the isValidSignatureWithSender function
│ ├── it should succeed with a valid signature
│ └── it should fail with an invalid signature
│ └── it should succeed with a valid signature for isValidSignatureWithSender
│ └── it should fail with an invalid 's' value for isValidSignatureWithSender
│ └── it should fail with an inverted 's' value for isValidSignatureWithSender
├── when testing the transferOwnership function
│ ├── it should transfer ownership to a new address
│ └── it should revert when transferring to the zero address
Expand All @@ -27,3 +28,11 @@ TestK1Validator
└── when testing the isModuleType function
├── it should return true for VALIDATOR module type
└── it should return false for an invalid module type
├── when testing the addSafeSender function
│ └── it should add a safe sender to the safe senders list
├── when testing the removeSafeSender function
│ └── it should remove a safe sender from the safe senders list
├── when testing the fillSafeSenders function
│ └── it should fill the safe senders list
├── when testing the isSafeSender function
│ └── it should check if a sender is in the safe senders list

0 comments on commit d015436

Please sign in to comment.