diff --git a/.gas-snapshot b/.gas-snapshot index 2e5a63d..224c255 100644 --- a/.gas-snapshot +++ b/.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) \ No newline at end of file +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) \ No newline at end of file diff --git a/src/CoinbaseSmartWallet.sol b/src/CoinbaseSmartWallet.sol index e6042c5..4556be1 100644 --- a/src/CoinbaseSmartWallet.sol +++ b/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 diff --git a/src/MultiOwnable.sol b/src/MultiOwnable.sol index 70437c1..eca5f67 100644 --- a/src/MultiOwnable.sol +++ b/src/MultiOwnable.sol @@ -113,7 +113,7 @@ contract MultiOwnable { } function _getMultiOwnableStorage() internal pure returns (MultiOwnableStorage storage $) { - assembly { + assembly ("memory-safe") { $.slot := MultiOwnableStorageLocation } } diff --git a/test/CoinbaseSmartWallet/ExecuteWithoutChainIdValidation.t.sol b/test/CoinbaseSmartWallet/ExecuteWithoutChainIdValidation.t.sol index 8baa3e2..3c2084d 100644 --- a/test/CoinbaseSmartWallet/ExecuteWithoutChainIdValidation.t.sol +++ b/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); } diff --git a/test/CoinbaseSmartWallet/IsValidSignature.t.sol b/test/CoinbaseSmartWallet/IsValidSignature.t.sol index d55b362..156934c 100644 --- a/test/CoinbaseSmartWallet/IsValidSignature.t.sol +++ b/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,10 +153,8 @@ 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 { @@ -140,9 +162,7 @@ contract TestIsValidSignature is SmartWalletTestBase { 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))); } }