Skip to content

Commit

Permalink
fix: [ALC-LA2-2] revert on malformed signature (alchemyplatform#48)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jaypaik
jaypaik authored Apr 5, 2024
1 parent 93f46a2 commit 3648cb0
Show file tree
Hide file tree
Showing 7 changed files with 154 additions and 46 deletions.
20 changes: 9 additions & 11 deletions src/LightAccount.sol
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.23;

/* solhint-disable avoid-low-level-calls */
/* solhint-disable no-inline-assembly */
/* solhint-disable reason-string */

import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol";
import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol";
Expand Down Expand Up @@ -124,6 +120,9 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable {
override
returns (uint256 validationData)
{
if (userOp.signature.length < 1) {
revert InvalidSignatureType();
}
uint8 signatureType = uint8(userOp.signature[0]);
if (signatureType == uint8(SignatureType.EOA)) {
// EOA signature
Expand All @@ -135,18 +134,17 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable {
bytes memory signature = userOp.signature[1:];
return _successToValidationData(_isValidContractOwnerSignatureNow(userOpHash, signature));
}

revert InvalidSignatureType();
}

/// @notice Check if the signature is a valid by the EOA owner for the given digest.
/// @dev Only supports 65-byte signatures, and uses the digest directly.
/// @dev Only supports 65-byte signatures, and uses the digest directly. Reverts if the signature is malformed.
/// @param digest The digest to be checked.
/// @param signature The signature to be checked.
/// @return True if the signature is valid and by the owner, false otherwise.
function _isValidEOAOwnerSignature(bytes32 digest, bytes memory signature) internal view returns (bool) {
(address recovered, ECDSA.RecoverError error,) = digest.tryRecover(signature);
return error == ECDSA.RecoverError.NoError && recovered == owner();
address recovered = digest.recover(signature);
return recovered == owner();
}

/// @notice Check if the signature is a valid ERC-1271 signature by a contract owner for the given digest.
Expand All @@ -158,7 +156,7 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable {
}

/// @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).
/// 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)
internal
view
Expand All @@ -167,7 +165,7 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable {
returns (bool)
{
if (trimmedSignature.length < 1) {
return false;
revert InvalidSignatureType();
}
uint8 signatureType = uint8(trimmedSignature[0]);
if (signatureType == uint8(SignatureType.EOA)) {
Expand All @@ -179,7 +177,7 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable {
bytes memory signature = trimmedSignature[1:];
return _isValidContractOwnerSignatureNow(derivedHash, signature);
}
return false;
revert InvalidSignatureType();
}

function _domainNameAndVersion()
Expand Down
16 changes: 9 additions & 7 deletions src/MultiOwnerLightAccount.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,9 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable {
override
returns (uint256 validationData)
{
if (userOp.signature.length < 1) {
revert InvalidSignatureType();
}
uint8 signatureType = uint8(userOp.signature[0]);
if (signatureType == uint8(SignatureType.EOA)) {
// EOA signature
Expand All @@ -154,18 +157,17 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable {
return
_successToValidationData(_isValidContractOwnerSignatureNowSingle(contractOwner, userOpHash, signature));
}

revert InvalidSignatureType();
}

/// @notice Check if the signature is a valid by an EOA owner for the given digest.
/// @dev Only supports 65-byte signatures, and uses the digest directly.
/// @dev Only supports 65-byte signatures, and uses the digest directly. Reverts if the signature is malformed.
/// @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 _isValidEOAOwnerSignature(bytes32 digest, bytes memory signature) internal view returns (bool) {
(address recovered, ECDSA.RecoverError error,) = digest.tryRecover(signature);
return error == ECDSA.RecoverError.NoError && _getStorage().owners.contains(recovered.toSetValue());
address recovered = digest.recover(signature);
return _getStorage().owners.contains(recovered.toSetValue());
}

/// @notice Check if the given verifier is a contract owner, and if the signature is a valid ERC-1271 signature by
Expand Down Expand Up @@ -207,7 +209,7 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable {
}

/// @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).
/// 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)
internal
view
Expand All @@ -216,7 +218,7 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable {
returns (bool)
{
if (trimmedSignature.length < 1) {
return false;
revert InvalidSignatureType();
}
uint8 signatureType = uint8(trimmedSignature[0]);
if (signatureType == uint8(SignatureType.EOA)) {
Expand All @@ -233,7 +235,7 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable {
bytes memory signature = trimmedSignature[21:];
return _isValidContractOwnerSignatureNowSingle(contractOwner, derivedHash, signature);
}
return false;
revert InvalidSignatureType();
}

function _domainNameAndVersion()
Expand Down
2 changes: 1 addition & 1 deletion test/CustomSlotInitializable.t.sol
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.23;

import "forge-std/Test.sol";
Expand Down
84 changes: 73 additions & 11 deletions test/LightAccount.t.sol
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.23;

import "forge-std/Test.sol";

import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol";
import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol";

import {EntryPoint} from "account-abstraction/core/EntryPoint.sol";
import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol";
import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOperation.sol";
Expand Down Expand Up @@ -113,6 +112,47 @@ contract LightAccountTest is Test {
entryPoint.handleOps(ops, BENEFICIARY);
}

function testRevertsUserOpsWithMalformedSignature() public {
PackedUserOperation memory op = _getSignedOp(
abi.encodeCall(BaseLightAccount.execute, (address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()))),
1234
);
op.signature = abi.encodePacked(BaseLightAccount.SignatureType.EOA, hex"aaaa");
PackedUserOperation[] memory ops = new PackedUserOperation[](1);
ops[0] = op;
vm.expectRevert(
abi.encodeWithSelector(
IEntryPoint.FailedOpWithRevert.selector,
0,
"AA23 reverted",
abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (op.signature.length - 1))
)
);
entryPoint.handleOps(ops, BENEFICIARY);

op.signature = abi.encodePacked(uint8(3));
vm.expectRevert(
abi.encodeWithSelector(
IEntryPoint.FailedOpWithRevert.selector,
0,
"AA23 reverted",
abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)
)
);
entryPoint.handleOps(ops, BENEFICIARY);

op.signature = hex"";
vm.expectRevert(
abi.encodeWithSelector(
IEntryPoint.FailedOpWithRevert.selector,
0,
"AA23 reverted",
abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)
)
);
entryPoint.handleOps(ops, BENEFICIARY);
}

function testExecuteCannotBeCalledByRandos() public {
vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.NotAuthorized.selector, (address(this))));
account.execute(address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()));
Expand Down Expand Up @@ -334,16 +374,30 @@ contract LightAccountTest is Test {

function testIsValidSignatureRejectsInvalid() public {
bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world"));
bytes memory signature =
abi.encodePacked(_sign(123, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child);
bytes memory signature = abi.encodePacked(
BaseLightAccount.SignatureType.EOA,
_sign(123, _toERC1271Hash(child)),
_PARENT_TYPEHASH,
_domainSeparatorB(),
child
);
assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff));

signature = abi.encodePacked(
_sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorA(), child
BaseLightAccount.SignatureType.EOA,
_sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)),
_PARENT_TYPEHASH,
_domainSeparatorA(),
child
);
assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff));

assertEq(account.isValidSignature(_toChildHash(child), ""), bytes4(0xffffffff));
// ERC1271.isValidSignature only truncates 32 bytes (since the wrong domain separator was used) before passing it on to _isValidSignature
// _isValidSignature truncates the SignatureType byte before ecrecover
vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1)));
account.isValidSignature(_toChildHash(child), signature);

vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector));
account.isValidSignature(_toChildHash(child), abi.encodePacked(BaseLightAccount.SignatureType.EOA));
}

function testIsValidSignaturePersonalSign() public {
Expand Down Expand Up @@ -375,18 +429,26 @@ contract LightAccountTest is Test {
string memory message = "hello world";
bytes32 childHash =
keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message));
bytes memory signature = abi.encodePacked(_sign(123, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH);
bytes memory signature = abi.encodePacked(
BaseLightAccount.SignatureType.EOA, _sign(123, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH
);
assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff));

signature = abi.encodePacked(
BaseLightAccount.SignatureType.EOA,
_sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)),
_PARENT_TYPEHASH,
_domainSeparatorB(),
childHash
);
assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff));

assertEq(account.isValidSignature(childHash, ""), bytes4(0xffffffff));
// ERC1271.isValidSignature only truncates 32 bytes for personal_sign before passing it on to _isValidSignature
// _isValidSignature truncates the SignatureType byte before ecrecover
vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1)));
account.isValidSignature(childHash, signature);

vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector));
account.isValidSignature(childHash, "");
}

function testOwnerCanUpgrade() public {
Expand Down Expand Up @@ -487,7 +549,7 @@ contract LightAccountTest is Test {
bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032)))
)
),
0x10f16a2b2ab8a4a6c680a35494382da995eea00c40bed1fc5391774da7de7fc9
0x9861f47fa11c7176759734bb81b2c2c764ce9219f757a6aa77774550dfe37f33
);
}

Expand Down
2 changes: 1 addition & 1 deletion test/LightAccountFactory.t.sol
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.23;

import "forge-std/Test.sol";
Expand Down
74 changes: 60 additions & 14 deletions test/MultiOwnerLightAccount.t.sol
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.23;

import "forge-std/Test.sol";
Expand Down Expand Up @@ -163,6 +163,47 @@ contract MultiOwnerLightAccountTest is Test {
entryPoint.handleOps(ops, BENEFICIARY);
}

function testRevertsUserOpsWithMalformedSignature() public {
PackedUserOperation memory op = _getSignedOp(
abi.encodeCall(BaseLightAccount.execute, (address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()))),
1234
);
op.signature = abi.encodePacked(BaseLightAccount.SignatureType.EOA, hex"aaaa");
PackedUserOperation[] memory ops = new PackedUserOperation[](1);
ops[0] = op;
vm.expectRevert(
abi.encodeWithSelector(
IEntryPoint.FailedOpWithRevert.selector,
0,
"AA23 reverted",
abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (op.signature.length - 1))
)
);
entryPoint.handleOps(ops, BENEFICIARY);

op.signature = abi.encodePacked(uint8(3));
vm.expectRevert(
abi.encodeWithSelector(
IEntryPoint.FailedOpWithRevert.selector,
0,
"AA23 reverted",
abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)
)
);
entryPoint.handleOps(ops, BENEFICIARY);

op.signature = hex"";
vm.expectRevert(
abi.encodeWithSelector(
IEntryPoint.FailedOpWithRevert.selector,
0,
"AA23 reverted",
abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)
)
);
entryPoint.handleOps(ops, BENEFICIARY);
}

function testExecuteCannotBeCalledByRandos() public {
vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.NotAuthorized.selector, (address(this))));
account.execute(address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()));
Expand Down Expand Up @@ -469,12 +510,14 @@ contract MultiOwnerLightAccountTest is Test {
_domainSeparatorA(),
child
);
assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff));

assertEq(
account.isValidSignature(_toChildHash(child), abi.encodePacked(BaseLightAccount.SignatureType.EOA)),
bytes4(0xffffffff)
);
// ERC1271.isValidSignature only truncates 32 bytes (since the wrong domain separator was used) before passing it on to _isValidSignature
// _isValidSignature truncates the SignatureType byte before ecrecover
vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1)));
account.isValidSignature(_toChildHash(child), signature);

vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector));
account.isValidSignature(_toChildHash(child), abi.encodePacked(BaseLightAccount.SignatureType.EOA));
}

function testIsValidSignatureRejectsInvalidContractOwner() public {
Expand All @@ -490,14 +533,15 @@ contract MultiOwnerLightAccountTest is Test {
assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff));
}

function testFuzz_IsValidSignatureRejectsInvalidSignatureType(uint8 signatureType) public {
function testFuzz_isValidSignatureRejectsInvalidSignatureType(uint8 signatureType) public {
signatureType = uint8(bound(signatureType, 3, type(uint8).max));

bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world"));
bytes memory signature = abi.encodePacked(
signatureType, _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child
);
assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff));
vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector));
account.isValidSignature(_toChildHash(child), signature);
}

function testIsValidSignaturePersonalSign() public {
Expand Down Expand Up @@ -555,12 +599,14 @@ contract MultiOwnerLightAccountTest is Test {
_domainSeparatorB(),
childHash
);
assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff));

assertEq(
account.isValidSignature(childHash, abi.encodePacked(BaseLightAccount.SignatureType.EOA)),
bytes4(0xffffffff)
);
// ERC1271.isValidSignature only truncates 32 bytes for personal_sign before passing it on to _isValidSignature
// _isValidSignature truncates the SignatureType byte before ecrecover
vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1)));
account.isValidSignature(childHash, signature);

vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector));
account.isValidSignature(childHash, abi.encodePacked(BaseLightAccount.SignatureType.EOA));
}

function testOwnerCanUpgrade() public {
Expand Down Expand Up @@ -662,7 +708,7 @@ contract MultiOwnerLightAccountTest is Test {
bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032)))
)
),
0x04ef3a9310d1a2d867807831aa0361b20aad25421de58cf37457fcf7f9567b6a
0xff5809a60394ff57666c1f7b34605ab8a7fe83c99b833b484ed65f4fdd479c4e
);
}

Expand Down
Loading

0 comments on commit 3648cb0

Please sign in to comment.