Skip to content

Commit

Permalink
Support for new for EIP-1271 verification (#572)
Browse files Browse the repository at this point in the history 
* update 1271 in Account.sol

* add natspec and extend to AccountExtension.sol

* remove unsued deps

* update test

* remove unnecessary encodeMessage

---------

Co-authored-by: WhiteOakKong <aadam.debevec@outlook.com>
  • Loading branch information
@nkrishang @WhiteOakKong
nkrishang and WhiteOakKong authored Nov 13, 2023
1 parent e511612 commit a050a1a
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 8 deletions.
17 changes: 15 additions & 2 deletions contracts/prebuilts/account/non-upgradeable/Account.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ contract Account is AccountCore, ContractMetadata, ERC1271, ERC721Holder, ERC115
using ECDSA for bytes32;
using EnumerableSet for EnumerableSet.AddressSet;

bytes32 private constant MSG_TYPEHASH = keccak256("AccountMessage(bytes message)");

/*///////////////////////////////////////////////////////////////
Constructor, Initializer, Modifiers
//////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -61,14 +63,15 @@ contract Account is AccountCore, ContractMetadata, ERC1271, ERC721Holder, ERC115
}

/// @notice See EIP-1271
function isValidSignature(bytes32 _hash, bytes memory _signature)
function isValidSignature(bytes32 _message, bytes memory _signature)
public
view
virtual
override
returns (bytes4 magicValue)
{
address signer = _hash.recover(_signature);
bytes32 messageHash = getMessageHash(abi.encode(_message));
address signer = messageHash.recover(_signature);

if (isAdmin(signer)) {
return MAGICVALUE;
Expand All @@ -87,6 +90,16 @@ contract Account is AccountCore, ContractMetadata, ERC1271, ERC721Holder, ERC115
}
}

/**
* @notice Returns the hash of message that should be signed for EIP1271 verification.
* @param message Message to be hashed i.e. `keccak256(abi.encode(data))`
* @return Hashed message
*/
function getMessageHash(bytes memory message) public view returns (bytes32) {
bytes32 messageHash = keccak256(abi.encode(MSG_TYPEHASH, keccak256(message)));
return keccak256(abi.encodePacked("\x19\x01", _domainSeparatorV4(), messageHash));
}

/*///////////////////////////////////////////////////////////////
External functions
//////////////////////////////////////////////////////////////*/
Expand Down
17 changes: 15 additions & 2 deletions contracts/prebuilts/account/utils/AccountExtension.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ contract AccountExtension is ContractMetadata, ERC1271, AccountPermissions, ERC7
using ECDSA for bytes32;
using EnumerableSet for EnumerableSet.AddressSet;

bytes32 private constant MSG_TYPEHASH = keccak256("AccountMessage(bytes message)");

/*///////////////////////////////////////////////////////////////
Constructor, Initializer, Modifiers
//////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -63,14 +65,15 @@ contract AccountExtension is ContractMetadata, ERC1271, AccountPermissions, ERC7
}

/// @notice See EIP-1271
function isValidSignature(bytes32 _hash, bytes memory _signature)
function isValidSignature(bytes32 _message, bytes memory _signature)
public
view
virtual
override
returns (bytes4 magicValue)
{
address signer = _hash.recover(_signature);
bytes32 messageHash = getMessageHash(abi.encode(_message));
address signer = messageHash.recover(_signature);

if (isAdmin(signer)) {
return MAGICVALUE;
Expand All @@ -89,6 +92,16 @@ contract AccountExtension is ContractMetadata, ERC1271, AccountPermissions, ERC7
}
}

/**
* @notice Returns the hash of message that should be signed for EIP1271 verification.
* @param message Message to be hashed i.e. `keccak256(abi.encode(data))`
* @return Hashed message
*/
function getMessageHash(bytes memory message) public view returns (bytes32) {
bytes32 messageHash = keccak256(abi.encode(MSG_TYPEHASH, keccak256(message)));
return keccak256(abi.encodePacked("\x19\x01", _domainSeparatorV4(), messageHash));
}

/*///////////////////////////////////////////////////////////////
External functions
//////////////////////////////////////////////////////////////*/
Expand Down
35 changes: 31 additions & 4 deletions src/test/smart-wallet/AccountVulnPOC.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { TWProxy } from "contracts/infra/TWProxy.sol";

// Target
import { IAccountPermissions } from "contracts/extension/interface/IAccountPermissions.sol";
import { AccountFactory, Account } from "contracts/prebuilts/account/non-upgradeable/AccountFactory.sol";
import { AccountFactory, Account as SimpleAccount } from "contracts/prebuilts/account/non-upgradeable/AccountFactory.sol";

library GPv2EIP1271 {
bytes4 internal constant MAGICVALUE = 0x1626ba7e;
Expand Down Expand Up @@ -253,6 +253,8 @@ contract SimpleAccountVulnPOCTest is BaseTest {
/*//////////////////////////////////////////////////////////
Setup
//////////////////////////////////////////////////////////////*/
address account = accountFactory.getAddress(accountAdmin, bytes(""));

address[] memory approvedTargets = new address[](1);
approvedTargets[0] = address(0x123); // allowing accountSigner permissions for some random contract, consider it as 0 address here

Expand All @@ -270,7 +272,6 @@ contract SimpleAccountVulnPOCTest is BaseTest {

vm.prank(accountAdmin);
bytes memory sig = _signSignerPermissionRequest(permissionsReq);
address account = accountFactory.getAddress(accountAdmin, bytes(""));
IAccountPermissions(payable(account)).setPermissionsForSigner(permissionsReq, sig);

// As expected, Account Signer is not be able to call setNum on numberContract since it doesnt have numberContract as approved target
Expand All @@ -292,14 +293,40 @@ contract SimpleAccountVulnPOCTest is BaseTest {
Attack
//////////////////////////////////////////////////////////////*/

//However they can bypass this by using signature verification on number contract instead
// However they can bypass this by using signature verification on number contract instead
vm.prank(accountSigner);
bytes32 digest = keccak256(abi.encode(42));
(uint8 v, bytes32 r, bytes32 s) = vm.sign(accountSignerPKey, digest);
bytes32 toSign = SimpleAccount(payable(account)).getMessageHash(abi.encode(digest));
(uint8 v, bytes32 r, bytes32 s) = vm.sign(accountSignerPKey, toSign);
bytes memory signature = abi.encodePacked(r, s, v);

vm.expectRevert("Account: caller not approved target.");
numberContract.setNumBySignature(account, 42, signature);
assertEq(numberContract.num(), 0);

// Signer can perform transaction if target is approved.
address[] memory newApprovedTargets = new address[](2);
newApprovedTargets[0] = address(0x123); // allowing accountSigner permissions for some random contract, consider it as 0 address here
newApprovedTargets[1] = address(numberContract);

IAccountPermissions.SignerPermissionRequest memory updatedPermissionsReq = IAccountPermissions
.SignerPermissionRequest(
accountSigner,
0,
newApprovedTargets,
1 ether,
0,
type(uint128).max,
0,
type(uint128).max,
bytes32("another UID")
);

vm.prank(accountAdmin);
bytes memory sig2 = _signSignerPermissionRequest(updatedPermissionsReq);
IAccountPermissions(payable(account)).setPermissionsForSigner(updatedPermissionsReq, sig2);

numberContract.setNumBySignature(account, 42, signature);
assertEq(numberContract.num(), 42);
}
}

0 comments on commit a050a1a

Please sign in to comment.