Merge pull request #13 from coinbase/wilson/update-validate-signature

.gas-snapshot

@@ -29,21 +29,22 @@ TestCanSkipChainIdValidation:test_approvedSelectorsReturnTrue() (gas: 15889)
 TestCanSkipChainIdValidation:test_otherSelectorsReturnFalse() (gas: 12491)
 TestExecuteWithoutChainIdValidation:testExecute() (gas: 424683)
 TestExecuteWithoutChainIdValidation:testExecuteBatch() (gas: 729020)
-TestExecuteWithoutChainIdValidation:testExecuteBatch(uint256) (runs: 256, μ: 3419049, ~: 3273850)
-TestExecuteWithoutChainIdValidation:test__codesize() (gas: 48980)
-TestExecuteWithoutChainIdValidation:test__codesize() (gas: 49215)
-TestExecuteWithoutChainIdValidation:test_canChangeOwnerWithoutChainId() (gas: 287952)
-TestExecuteWithoutChainIdValidation:test_cannotCallExec() (gas: 220082)
+TestExecuteWithoutChainIdValidation:testExecuteBatch(uint256) (runs: 256, μ: 3376320, ~: 3249240)
+TestExecuteWithoutChainIdValidation:test__codesize() (gas: 48858)
+TestExecuteWithoutChainIdValidation:test__codesize() (gas: 49093)
+TestExecuteWithoutChainIdValidation:test_canChangeOwnerWithoutChainId() (gas: 287917)
+TestExecuteWithoutChainIdValidation:test_cannotCallExec() (gas: 220047)
 TestExecuteWithoutChainIdValidation:test_revertsIfCallerNotEntryPoint() (gas: 8620)
 TestExecuteWithoutChainIdValidation:test_revertsIfWrongNonceKey() (gas: 62275)
 TestExecuteWithoutChainIdValidation:test_revertsWithReservedNonce() (gas: 82302)
 TestInitialize:testInitialize() (gas: 21122)
-TestInitialize:test_cannotInitImplementation() (gas: 2703015)
-TestIsValidSignature:testRevertsIfEthereumSignatureButWrongOwnerLength() (gas: 31912)
-TestIsValidSignature:testRevertsIfPasskeySigButWrongOwnerLength() (gas: 40001)
-TestIsValidSignature:testValidateSignatureWithEOASigner() (gas: 25019)
-TestIsValidSignature:testValidateSignatureWithEOASignerFailsWithWrongSigner() (gas: 23860)
-TestIsValidSignature:testValidateSignatureWithPasskeySigner() (gas: 421298)
-TestIsValidSignature:testValidateSignatureWithPasskeySignerFailsBadOwnerIndex() (gas: 35149)
-TestIsValidSignature:testValidateSignatureWithPasskeySignerFailsWithWrongBadSignature() (gas: 428720)
-TestValidateUserOp:testValidateUserOp() (gas: 447183)
+TestInitialize:test_cannotInitImplementation() (gas: 2678562)
+TestIsValidSignature:testReturnsInvalidIfPasskeySigButWrongOwnerLength() (gas: 39464)
+TestIsValidSignature:testRevertsIfEthereumSignatureButWrongOwnerLength() (gas: 24046)
+TestIsValidSignature:testSmartWalletSigner() (gas: 2951388)
+TestIsValidSignature:testValidateSignatureWithEOASigner() (gas: 24917)
+TestIsValidSignature:testValidateSignatureWithEOASignerFailsWithWrongSigner() (gas: 23847)
+TestIsValidSignature:testValidateSignatureWithPasskeySigner() (gas: 421251)
+TestIsValidSignature:testValidateSignatureWithPasskeySignerFailsBadOwnerIndex() (gas: 34964)
+TestIsValidSignature:testValidateSignatureWithPasskeySignerFailsWithWrongBadSignature() (gas: 428717)
+TestValidateUserOp:testValidateUserOp() (gas: 447113)

src/CoinbaseSmartWallet.sol

@@ -40,29 +40,9 @@ contract CoinbaseSmartWallet is MultiOwnable, UUPSUpgradeable, Receiver, ERC1271
     /// @dev Helps enforce sequential sequencing of replayable transactions.
     uint256 public constant REPLAYABLE_NONCE_KEY = 8453;
 
-    /// @notice Reverted during signature validation when the given signature length is invalid.
-    ///
-    /// @dev ECDSA signature are expected to be exactly 65 bytes long (the r, s and v values).
-    /// @dev WebAuthn encoded structs are expected to be at least 66 bytes long.
-    ///
-    /// @param length The invalid received signature length.
-    error InvalidSignatureLength(uint256 length);
-
     /// @notice Reverted when trying to re-initialize an account.
     error Initialized();
 
-    /// @notice Reverted during signature validation when the retrieved owner bytes can't be
-    ///         associated with the given signature.
-    ///
-    /// @dev ECDSA signatures must be associated with ethereum address padded
-    ///      to 32 bytes.
-    /// @dev WebAuthn authentications must be associated with a owner of length 64 bytes: the X, Y
-    ///      values of the Secp256r1 public key.
-    ///
-    /// @param ownerIndex The given owner index that was used to retrieve the associated owner.
-    /// @param owner      The invalid owner bytes retrieved.
-    error InvalidOwnerForSignature(uint256 ownerIndex, bytes owner);
-
     /// @notice Reverted when executing a `UserOperation` that requires the chain ID to be validated
     ///         but this validation has been omitted.
     ///
@@ -109,8 +89,7 @@ contract CoinbaseSmartWallet is MultiOwnable, UUPSUpgradeable, Receiver, ERC1271
     modifier payPrefund(uint256 missingAccountFunds) virtual {
         _;
 
-        /// @solidity memory-safe-assembly
-        assembly {
+        assembly ("memory-safe") {
             if missingAccountFunds {
                 // Ignore failure (it's EntryPoint's job to verify, not the account's).
                 pop(call(gas(), caller(), missingAccountFunds, codesize(), 0x00, codesize(), 0x00))
@@ -282,7 +261,7 @@ contract CoinbaseSmartWallet is MultiOwnable, UUPSUpgradeable, Receiver, ERC1271
     function _call(address target, uint256 value, bytes memory data) internal {
         (bool success, bytes memory result) = target.call{value: value}(data);
         if (!success) {
-            assembly {
+            assembly ("memory-safe") {
                 revert(add(result, 32), mload(result))
             }
         }
@@ -311,24 +290,22 @@ contract CoinbaseSmartWallet is MultiOwnable, UUPSUpgradeable, Receiver, ERC1271
         SignatureWrapper memory sigWrapper = abi.decode(wrappedSignatureBytes, (SignatureWrapper));
         bytes memory ownerBytes = ownerAtIndex(sigWrapper.ownerIndex);
 
-        if (sigWrapper.signatureData.length == 65) {
-            if (ownerBytes.length != 32) revert InvalidOwnerForSignature(sigWrapper.ownerIndex, ownerBytes);
+        if (ownerBytes.length == 32) {
             if (uint256(bytes32(ownerBytes)) > type(uint160).max) {
-                revert InvalidOwnerForSignature(sigWrapper.ownerIndex, ownerBytes);
+                // technically should be impossible given owners can only be added with
+                // addOwnerAddress and addOwnerPublicKey, but we leave incase of future changes.
+                revert InvalidEthereumAddressOwner(ownerBytes);
             }
 
             address owner;
-            /// @solidity memory-safe-assembly
-            assembly {
+            assembly ("memory-safe") {
                 owner := mload(add(ownerBytes, 32))
             }
+
             return SignatureCheckerLib.isValidSignatureNow(owner, message, sigWrapper.signatureData);
         }
 
-        // Passkey signature
-        if (sigWrapper.signatureData.length > 65) {
-            if (ownerBytes.length != 64) revert InvalidOwnerForSignature(sigWrapper.ownerIndex, ownerBytes);
-
+        if (ownerBytes.length == 64) {
             (uint256 x, uint256 y) = abi.decode(ownerBytes, (uint256, uint256));
 
             WebAuthn.WebAuthnAuth memory auth = abi.decode(sigWrapper.signatureData, (WebAuthn.WebAuthnAuth));
@@ -342,7 +319,7 @@ contract CoinbaseSmartWallet is MultiOwnable, UUPSUpgradeable, Receiver, ERC1271
             });
         }
 
-        revert InvalidSignatureLength(sigWrapper.signatureData.length);
+        revert InvalidOwnerBytesLength(ownerBytes);
     }
 
     /// @inheritdoc UUPSUpgradeable

src/MultiOwnable.sol

@@ -117,7 +117,7 @@ contract MultiOwnable {
     }
 
     function _getMultiOwnableStorage() internal pure returns (MultiOwnableStorage storage $) {
-        assembly {
+        assembly ("memory-safe") {
             $.slot := MultiOwnableStorageLocation
         }
     }

test/CoinbaseSmartWallet/ExecuteWithoutChainIdValidation.t.sol

@@ -42,7 +42,7 @@ contract TestExecuteWithoutChainIdValidation is SmartWalletTestBase {
         UserOperation memory userOp = _getUserOpWithSignature();
         vm.expectEmit(true, true, true, true);
         emit IEntryPoint.UserOperationEvent(
-            entryPoint.getUserOpHash(userOp), userOp.sender, address(0), userOp.nonce, false, 0, 48062
+            entryPoint.getUserOpHash(userOp), userOp.sender, address(0), userOp.nonce, false, 0, 48027
         );
         _sendUserOperation(userOp);
     }

test/CoinbaseSmartWallet/IsValidSignature.t.sol

@@ -32,6 +32,30 @@ contract TestIsValidSignature is SmartWalletTestBase {
         assertEq(ret, bytes4(0x1626ba7e));
     }
 
+    function testSmartWalletSigner() public {
+        MockCoinbaseSmartWallet otherAccount = new MockCoinbaseSmartWallet();
+        otherAccount.initialize(owners);
+
+        vm.prank(signer);
+        account.addOwnerAddress(address(otherAccount));
+
+        bytes32 hash = 0x15fa6f8c855db1dccbb8a42eef3a7b83f11d29758e84aed37312527165d5eec5;
+        bytes32 toSign = account.replaySafeHash(hash);
+        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, otherAccount.replaySafeHash(toSign));
+        bytes memory signature = abi.encodePacked(r, s, v);
+
+        CoinbaseSmartWallet.SignatureWrapper memory wrapperForOtherAccount =
+            CoinbaseSmartWallet.SignatureWrapper(0, signature);
+
+        bytes memory sig = abi.encode(
+            CoinbaseSmartWallet.SignatureWrapper({ownerIndex: 2, signatureData: abi.encode(wrapperForOtherAccount)})
+        );
+
+        // check a valid signature
+        bytes4 ret = account.isValidSignature(hash, sig);
+        assertEq(ret, bytes4(0x1626ba7e));
+    }
+
     function testValidateSignatureWithPasskeySignerFailsBadOwnerIndex() public {
         bytes32 hash = 0x15fa6f8c855db1dccbb8a42eef3a7b83f11d29758e84aed37312527165d5eec5;
         bytes32 challenge = account.replaySafeHash(hash);
@@ -56,7 +80,7 @@ contract TestIsValidSignature is SmartWalletTestBase {
             })
         );
 
-        vm.expectRevert(abi.encodeWithSelector(CoinbaseSmartWallet.InvalidOwnerForSignature.selector, uint8(2), ""));
+        vm.expectRevert(abi.encodeWithSelector(MultiOwnable.InvalidOwnerBytesLength.selector, hex""));
         account.isValidSignature(hash, sig);
     }
 
@@ -105,7 +129,7 @@ contract TestIsValidSignature is SmartWalletTestBase {
         assertEq(ret, bytes4(0xffffffff));
     }
 
-    function testRevertsIfPasskeySigButWrongOwnerLength() public {
+    function testReturnsInvalidIfPasskeySigButWrongOwnerLength() public {
         bytes32 hash = 0x15fa6f8c855db1dccbb8a42eef3a7b83f11d29758e84aed37312527165d5eec5;
         bytes32 challenge = account.replaySafeHash(hash);
         WebAuthnInfo memory webAuthn = Utils.getWebAuthnStruct(challenge);
@@ -129,20 +153,16 @@ contract TestIsValidSignature is SmartWalletTestBase {
             })
         );
 
-        vm.expectRevert(
-            abi.encodeWithSelector(CoinbaseSmartWallet.InvalidOwnerForSignature.selector, uint8(0), abi.encode(signer))
-        );
-        account.isValidSignature(hash, sig);
+        bytes4 ret = account.isValidSignature(hash, sig);
+        assertEq(ret, bytes4(0xffffffff));
     }
 
     function testRevertsIfEthereumSignatureButWrongOwnerLength() public {
         bytes32 hash = 0x15fa6f8c855db1dccbb8a42eef3a7b83f11d29758e84aed37312527165d5eec5;
         bytes32 toSign = SignatureCheckerLib.toEthSignedMessageHash(account.replaySafeHash(hash));
         (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, toSign);
         bytes memory signature = abi.encodePacked(r, s, v);
-        vm.expectRevert(
-            abi.encodeWithSelector(CoinbaseSmartWallet.InvalidOwnerForSignature.selector, uint8(1), passkeyOwner)
-        );
+        vm.expectRevert();
         account.isValidSignature(hash, abi.encode(CoinbaseSmartWallet.SignatureWrapper(1, signature)));
     }
 }