Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: also exclude this case from isValidSignature
Browse files Browse the repository at this point in the history
jaypaik committed Apr 5, 2024
1 parent 6b0f5d7 commit cb2548d
Showing 2 changed files with 20 additions and 49 deletions.
35 changes: 3 additions & 32 deletions src/MultiOwnerLightAccount.sol
Original file line number Diff line number Diff line change
@@ -130,7 +130,6 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable {
/// @dev Implement template method of BaseAccount.
/// Uses a modified version of `SignatureChecker.isValidSignatureNow` in which the digest is wrapped with an
/// "Ethereum Signed Message" envelope for the EOA-owner case but not in the ERC-1271 contract-owner case.
/// The SignatureType.CONTRACT case is excluded here to prevent potential gas griefing attacks.
function _validateSignature(PackedUserOperation calldata userOp, bytes32 userOpHash)
internal
virtual
@@ -150,8 +149,7 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable {
// Contract signature with address
address contractOwner = address(bytes20(userOp.signature[1:21]));
bytes memory signature = userOp.signature[21:];
return
_successToValidationData(_isValidContractOwnerSignatureNowSingle(contractOwner, userOpHash, signature));
return _successToValidationData(_isValidContractOwnerSignatureNow(contractOwner, userOpHash, signature));
}
revert InvalidSignatureType();
}
@@ -172,7 +170,7 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable {
/// @param digest The digest to be checked.
/// @param signature The signature to be checked.
/// @return True if the signature is valid and by an owner, false otherwise.
function _isValidContractOwnerSignatureNowSingle(address contractOwner, bytes32 digest, bytes memory signature)
function _isValidContractOwnerSignatureNow(address contractOwner, bytes32 digest, bytes memory signature)
internal
view
returns (bool)
@@ -181,29 +179,6 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable {
&& _getStorage().owners.contains(contractOwner.toSetValue());
}

/// @notice Check if the signature is a valid ERC-1271 signature by a contract owner for the given digest by
/// checking all owners in a loop.
/// @dev Susceptible to denial-of-service by a malicious owner contract. To avoid this, use a signature type that
/// includes the owner address.
/// @param digest The digest to be checked.
/// @param signature The signature to be checked.
/// @return True if the signature is valid and by an owner, false otherwise.
function _isValidContractOwnerSignatureNowLoop(bytes32 digest, bytes memory signature)
internal
view
returns (bool)
{
LightAccountStorage storage _storage = _getStorage();
address[] memory owners_ = _storage.owners.getAll().toAddressArray();
uint256 length = owners_.length;
for (uint256 i = 0; i < length; ++i) {
if (SignatureChecker.isValidERC1271SignatureNow(owners_[i], digest, signature)) {
return true;
}
}
return false;
}

/// @dev The signature is valid if it is signed by the owner's private key (if the owner is an EOA) or if it is a
/// valid ERC-1271 signature from the owner (if the owner is a contract). Reverts if the signature is malformed.
function _isValidSignature(bytes32 derivedHash, bytes calldata trimmedSignature)
@@ -221,15 +196,11 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable {
// EOA signature
bytes memory signature = trimmedSignature[1:];
return _isValidEOAOwnerSignature(derivedHash, signature);
} else if (signatureType == uint8(SignatureType.CONTRACT)) {
// Contract signature without address
bytes memory signature = trimmedSignature[1:];
return _isValidContractOwnerSignatureNowLoop(derivedHash, signature);
} else if (signatureType == uint8(SignatureType.CONTRACT_WITH_ADDR)) {
// Contract signature with address
address contractOwner = address(bytes20(trimmedSignature[1:21]));
bytes memory signature = trimmedSignature[21:];
return _isValidContractOwnerSignatureNowSingle(contractOwner, derivedHash, signature);
return _isValidContractOwnerSignatureNow(contractOwner, derivedHash, signature);
}
revert InvalidSignatureType();
}
34 changes: 17 additions & 17 deletions test/MultiOwnerLightAccount.t.sol
Original file line number Diff line number Diff line change
@@ -474,11 +474,12 @@ contract MultiOwnerLightAccountTest is Test {
);
}

function testIsValidSignatureForContractOwnerUnspecified() public {
function testIsValidSignatureForContractOwnerSpecified() public {
_useContractOwner();
bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world"));
bytes memory signature = abi.encodePacked(
BaseLightAccount.SignatureType.CONTRACT,
BaseLightAccount.SignatureType.CONTRACT_WITH_ADDR,
contractOwner,
contractOwner.sign(_toERC1271Hash(child)),
_PARENT_TYPEHASH,
_domainSeparatorB(),
@@ -490,21 +491,18 @@ contract MultiOwnerLightAccountTest is Test {
);
}

function testIsValidSignatureForContractOwnerSpecified() public {
function testIsValidSignatureRejectsContractOwnerUnspecified() public {
_useContractOwner();
bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world"));
bytes memory signature = abi.encodePacked(
BaseLightAccount.SignatureType.CONTRACT_WITH_ADDR,
contractOwner,
BaseLightAccount.SignatureType.CONTRACT,
contractOwner.sign(_toERC1271Hash(child)),
_PARENT_TYPEHASH,
_domainSeparatorB(),
child
);
assertEq(
account.isValidSignature(_toChildHash(child), signature),
bytes4(keccak256("isValidSignature(bytes32,bytes)"))
);
vm.expectRevert(abi.encodePacked(BaseLightAccount.InvalidSignatureType.selector));
account.isValidSignature(_toChildHash(child), signature);
}

function testIsValidSignatureRejectsInvalidEOA() public {
@@ -539,7 +537,8 @@ contract MultiOwnerLightAccountTest is Test {
// Signature should fail, because the contract owner is not an owner
bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world"));
bytes memory signature = abi.encodePacked(
BaseLightAccount.SignatureType.CONTRACT,
BaseLightAccount.SignatureType.CONTRACT_WITH_ADDR,
contractOwner,
contractOwner.sign(_toERC1271Hash(child)),
_PARENT_TYPEHASH,
_domainSeparatorB(),
@@ -571,31 +570,32 @@ contract MultiOwnerLightAccountTest is Test {
assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)")));
}

function testIsValidSignaturePersonalSignForContractOwnerUnspecified() public {
function testIsValidSignaturePersonalSignForContractOwnerSpecified() public {
_useContractOwner();
string memory message = "hello world";
bytes32 childHash =
keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message));
bytes memory signature = abi.encodePacked(
BaseLightAccount.SignatureType.CONTRACT,
BaseLightAccount.SignatureType.CONTRACT_WITH_ADDR,
contractOwner,
contractOwner.sign(_toERC1271HashPersonalSign(childHash)),
_PARENT_TYPEHASH
);
assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)")));
}

function testIsValidSignaturePersonalSignForContractOwnerSpecified() public {
function testIsValidSignaturePersonalSignRejectsContractOwnerUnspecified() public {
_useContractOwner();
string memory message = "hello world";
bytes32 childHash =
keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message));
bytes memory signature = abi.encodePacked(
BaseLightAccount.SignatureType.CONTRACT_WITH_ADDR,
contractOwner,
BaseLightAccount.SignatureType.CONTRACT,
contractOwner.sign(_toERC1271HashPersonalSign(childHash)),
_PARENT_TYPEHASH
);
assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)")));
vm.expectRevert(abi.encodePacked(BaseLightAccount.InvalidSignatureType.selector));
account.isValidSignature(childHash, signature);
}

function testIsValidSignaturePersonalSignRejectsInvalid() public {
@@ -723,7 +723,7 @@ contract MultiOwnerLightAccountTest is Test {
bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032)))
)
),
0x2fff6cdec81f22cf680a4c06509a0e189084f530d5fdbfa5325940de2d89cbff
0xd5c53db2178734fbfa4566d827f4af4dbb897979875488640713b7b7d4689d1a
);
}

0 comments on commit cb2548d

Please sign in to comment.