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

fix: [spearbit-38] ERC-1271 signature validation with EOA owners of contract owners of MSCA #15

Merged
merged 2 commits into from
Dec 12, 2023
Merged

fix: [spearbit-38] ERC-1271 signature validation with EOA owners of contract owners of MSCA #15

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1454643
fix: [spearbit-38] ERC-1271 signature validation with EOA owners of c…
fangting-alchemy Dec 12, 2023
0dff5bb
clean up and add more comments in tests
fangting-alchemy Dec 12, 2023
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
37 changes: 17 additions & 20 deletions src/plugins/owner/MultiOwnerPlugin.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,18 +136,15 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271, EIP712 {
function isValidSignature(bytes32 digest, bytes memory signature) public view override returns (bytes4) {
bytes memory messageData = encodeMessageData(msg.sender, abi.encode(digest));
bytes32 messageHash = keccak256(messageData);

// try to recover through ECDSA
(address signer, ECDSA.RecoverError error) = ECDSA.tryRecover(messageHash, signature);
if (error == ECDSA.RecoverError.NoError) {
if (_owners.contains(msg.sender, CastLib.toSetValue(signer))) {
return _1271_MAGIC_VALUE;
} else {
return _1271_MAGIC_VALUE_FAILURE;
}
} else {
if (_isValidERC1271OwnerTypeSignature(msg.sender, messageHash, signature)) {
return _1271_MAGIC_VALUE;
}
if (error == ECDSA.RecoverError.NoError && _owners.contains(msg.sender, CastLib.toSetValue(signer))) {
return _1271_MAGIC_VALUE;
}

if (_isValidERC1271OwnerTypeSignature(msg.sender, messageHash, signature)) {
return _1271_MAGIC_VALUE;
}

return _1271_MAGIC_VALUE_FAILURE;
Expand Down Expand Up @@ -208,20 +205,20 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271, EIP712 {
override
returns (uint256)
{
(address signer, ECDSA.RecoverError error) =
(userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature);
if (functionId == uint8(FunctionId.USER_OP_VALIDATION_OWNER)) {
if (error == ECDSA.RecoverError.NoError) {
if (isOwnerOf(msg.sender, signer)) {
return _SIG_VALIDATION_PASSED;
}
} else {
if (_isValidERC1271OwnerTypeSignature(msg.sender, userOpHash, userOp.signature)) {
return _SIG_VALIDATION_PASSED;
}
(address signer, ECDSA.RecoverError error) =
(userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature);
fangting-alchemy marked this conversation as resolved.
Show resolved Hide resolved
if (error == ECDSA.RecoverError.NoError && isOwnerOf(msg.sender, signer)) {
return _SIG_VALIDATION_PASSED;
}

if (_isValidERC1271OwnerTypeSignature(msg.sender, userOpHash, userOp.signature)) {
return _SIG_VALIDATION_PASSED;
}

return _SIG_VALIDATION_FAILED;
}

revert NotImplemented();
}

Expand Down
14 changes: 13 additions & 1 deletion test/mocks/ContractOwner.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,30 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.21;

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

import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol";

contract ContractOwner is IERC1271 {
bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e;
address public owner;

constructor(address _owner) {
owner = _owner;
}

function sign(bytes32 digest) public pure returns (bytes memory) {
return abi.encodePacked("Signed: ", digest);
}

function isValidSignature(bytes32 digest, bytes memory signature) public pure override returns (bytes4) {
function isValidSignature(bytes32 digest, bytes memory signature) public view override returns (bytes4) {
fangting-alchemy marked this conversation as resolved.
Show resolved Hide resolved
if (keccak256(signature) == keccak256(sign(digest))) {
return _1271_MAGIC_VALUE;
} else {
(address signer,) = ECDSA.tryRecover(digest, signature);
if (signer == owner) {
return _1271_MAGIC_VALUE;
}
}
return 0xffffffff;
}
Expand Down
42 changes: 41 additions & 1 deletion test/plugin/owner/MultiOwnerPlugin.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ contract MultiOwnerPluginTest is Test {
address public owner1;
address public owner2;
address public owner3;
address public ownerofContractOwner;
uint256 public ownerofContractOwnerKey;
ContractOwner public contractOwner;
address[] public ownerArray;

Expand All @@ -41,7 +43,8 @@ contract MultiOwnerPluginTest is Test {
owner1 = makeAddr("owner1");
owner2 = makeAddr("owner2");
owner3 = makeAddr("owner3");
contractOwner = new ContractOwner();
(ownerofContractOwner, ownerofContractOwnerKey) = makeAddrAndKey("ownerofContractOwner");
contractOwner = new ContractOwner(ownerofContractOwner);

// set up owners for accountA
ownerArray = new address[](3);
Expand Down Expand Up @@ -172,6 +175,19 @@ contract MultiOwnerPluginTest is Test {
assertEq(_1271_MAGIC_VALUE, plugin.isValidSignature(digest, signature));
}

function testFuzz_isValidSignature_ContractOwnerWithEOAOwner(bytes32 digest) public {
address[] memory ownersToAdd = new address[](1);
ownersToAdd[0] = address(contractOwner);
plugin.updateOwners(ownersToAdd, new address[](0));

bytes32 messageDigest = plugin.getMessageHash(address(accountA), abi.encode(digest));
// owner3 is the EOA Owner of the contractOwner
// bytes memory signature = owner3.sign(messageDigest);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerofContractOwnerKey, messageDigest);
bytes memory signature = abi.encodePacked(r, s, v);
assertEq(_1271_MAGIC_VALUE, plugin.isValidSignature(digest, signature));
howydev marked this conversation as resolved.
Show resolved Hide resolved
}

function test_runtimeValidationFunction_OwnerOrSelf() public {
// should pass with owner as sender
plugin.runtimeValidationFunction(
Expand Down Expand Up @@ -210,6 +226,30 @@ contract MultiOwnerPluginTest is Test {
assertEq(resSuccess, 0);
}

function testFuzz_userOpValidationFunction_ContractOwnerWithEOAOwner(UserOperation memory userOp) public {
bytes32 userOpHash = entryPoint.getUserOpHash(userOp);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerofContractOwnerKey, userOpHash);

// sig cannot cover the whole userop struct since userop struct has sig field
userOp.signature = abi.encodePacked(r, s, v);

// should fail without owner access
uint256 resFail = plugin.userOpValidationFunction(
uint8(IMultiOwnerPlugin.FunctionId.USER_OP_VALIDATION_OWNER), userOp, userOpHash
);
assertEq(resFail, 1);

address[] memory ownersToAdd = new address[](1);
ownersToAdd[0] = address(contractOwner);
plugin.updateOwners(ownersToAdd, new address[](0));

// should pass with owner access
uint256 resSuccess = plugin.userOpValidationFunction(
uint8(IMultiOwnerPlugin.FunctionId.USER_OP_VALIDATION_OWNER), userOp, userOpHash
);
assertEq(resSuccess, 0);
}

function testFuzz_userOpValidationFunction_EOAOwner(string memory salt, UserOperation memory userOp) public {
// range bound the possible set of priv keys
(address signer, uint256 privateKey) = makeAddrAndKey(salt);
Expand Down