diff --git a/contracts/prebuilts/account/non-upgradeable/Account.sol b/contracts/prebuilts/account/non-upgradeable/Account.sol index 82212d517..5a34fdef0 100644 --- a/contracts/prebuilts/account/non-upgradeable/Account.sol +++ b/contracts/prebuilts/account/non-upgradeable/Account.sol @@ -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 //////////////////////////////////////////////////////////////*/ @@ -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; @@ -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 //////////////////////////////////////////////////////////////*/ diff --git a/contracts/prebuilts/account/utils/AccountExtension.sol b/contracts/prebuilts/account/utils/AccountExtension.sol index c3ec67a72..1661e5512 100644 --- a/contracts/prebuilts/account/utils/AccountExtension.sol +++ b/contracts/prebuilts/account/utils/AccountExtension.sol @@ -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 //////////////////////////////////////////////////////////////*/ @@ -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; @@ -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 //////////////////////////////////////////////////////////////*/ diff --git a/src/test/smart-wallet/AccountVulnPOC.t.sol b/src/test/smart-wallet/AccountVulnPOC.t.sol index 67df08504..0899b5ea9 100644 --- a/src/test/smart-wallet/AccountVulnPOC.t.sol +++ b/src/test/smart-wallet/AccountVulnPOC.t.sol @@ -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; @@ -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 @@ -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 @@ -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); } }