Support for new for EIP-1271 verification (#572)

* 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>

Author: nkrishang

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
     //////////////////////////////////////////////////////////////*/

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
     //////////////////////////////////////////////////////////////*/

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);
     }
 }