From d0173f0494ad84aaf71855f6a6c2f2fd79f284b5 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Fri, 9 Aug 2024 07:20:43 +0000 Subject: [PATCH 01/19] chore: migrate GPv2Signing domain separator tests to Foundry --- test/GPv2Signing.test.ts | 19 ------------ test/GPv2Signing/DomainSeparator.t.sol | 27 +++++++++++++++++ test/GPv2Signing/Helper.sol | 33 +++++++------------- test/libraries/Eip712.sol | 42 ++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 41 deletions(-) create mode 100644 test/GPv2Signing/DomainSeparator.t.sol diff --git a/test/GPv2Signing.test.ts b/test/GPv2Signing.test.ts index c6d8ec08..1a7c9b4b 100644 --- a/test/GPv2Signing.test.ts +++ b/test/GPv2Signing.test.ts @@ -38,25 +38,6 @@ describe("GPv2Signing", () => { testDomain = domain(chainId, signing.address); }); - describe("domainSeparator", () => { - it("should have an EIP-712 domain separator", async () => { - expect(await signing.domainSeparator()).to.equal( - ethers.utils._TypedDataEncoder.hashDomain(testDomain), - ); - }); - - it("should have a different replay protection for each deployment", async () => { - const GPv2Signing = await ethers.getContractFactory( - "GPv2SigningTestInterface", - ); - const signing2 = await GPv2Signing.deploy(); - - expect(await signing.domainSeparator()).to.not.equal( - await signing2.domainSeparator(), - ); - }); - }); - describe("setPreSignature", () => { const [owner, nonOwner] = traders; const orderUid = packOrderUidParams({ diff --git a/test/GPv2Signing/DomainSeparator.t.sol b/test/GPv2Signing/DomainSeparator.t.sol new file mode 100644 index 00000000..32949567 --- /dev/null +++ b/test/GPv2Signing/DomainSeparator.t.sol @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.8; + +import {Helper} from "./Helper.sol"; + +import {Harness, Helper} from "./Helper.sol"; +import {Eip712} from "test/libraries/Eip712.sol"; +import {Order as OrderLib} from "test/libraries/Order.sol"; + +contract DomainSeparator is Helper { + function test_TYPE_HASH_matches_the_EIP_712_order_type_hash() public view { + bytes32 expectedDomainSeparator = Eip712.hashStruct( + Eip712.EIP712Domain({ + name: "Gnosis Protocol", + version: "v2", + chainId: block.chainid, + verifyingContract: address(executor) + }) + ); + assertEq(executor.domainSeparator(), expectedDomainSeparator); + } + + function test_should_have_a_different_replay_protection_for_each_deployment() public { + Harness signing = new Harness(); + assertNotEq(executor.domainSeparator(), signing.domainSeparator()); + } +} diff --git a/test/GPv2Signing/Helper.sol b/test/GPv2Signing/Helper.sol index 1ff214f6..98e07eac 100644 --- a/test/GPv2Signing/Helper.sol +++ b/test/GPv2Signing/Helper.sol @@ -1,30 +1,19 @@ // SPDX-License-Identifier: LGPL-3.0-or-later pragma solidity ^0.8; -import {IERC20} from "src/contracts/interfaces/IERC20.sol"; -import {GPv2Order} from "src/contracts/libraries/GPv2Order.sol"; -import {GPv2Trade} from "src/contracts/libraries/GPv2Trade.sol"; -import {GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol"; +import {Test} from "forge-std/Test.sol"; -// solhint-disable func-name-mixedcase -contract Harness is GPv2Signing { - constructor(bytes32 _domainSeparator) { - domainSeparator = _domainSeparator; - } +import {GPv2SigningTestInterface} from "test/src/GPv2SigningTestInterface.sol"; - function exposed_recoverOrderFromTrade( - RecoveredOrder memory recoveredOrder, - IERC20[] calldata tokens, - GPv2Trade.Data calldata trade - ) external view { - recoverOrderFromTrade(recoveredOrder, tokens, trade); - } +// TODO: move the content of `GPv2SigningTestInterface` here once all tests have +// been removed. +// solhint-disable-next-line no-empty-blocks +contract Harness is GPv2SigningTestInterface {} + +contract Helper is Test { + Harness internal executor; - function exposed_recoverOrderSigner(GPv2Order.Data memory order, Scheme signingScheme, bytes calldata signature) - external - view - returns (bytes32 orderDigest, address owner) - { - (orderDigest, owner) = recoverOrderSigner(order, signingScheme, signature); + function setUp() public { + executor = new Harness(); } } diff --git a/test/libraries/Eip712.sol b/test/libraries/Eip712.sol index b387f701..985ace84 100644 --- a/test/libraries/Eip712.sol +++ b/test/libraries/Eip712.sol @@ -4,6 +4,13 @@ pragma solidity ^0.8; import {GPv2Order} from "src/contracts/libraries/GPv2Order.sol"; library Eip712 { + struct EIP712Domain { + string name; + string version; + uint256 chainId; + address verifyingContract; + } + // This is the struct representing an order that is signed by the user using // EIP-712. struct Order { @@ -21,6 +28,25 @@ library Eip712 { string buyTokenBalance; } + // Ideally, this would be replaced by type(EIP712Domain).typehash. + // Progress tracking for this Solidity feature is here: + // https://github.com/ethereum/solidity/issues/14157 + function EIP712DOMAIN_TYPE_HASH() internal pure returns (bytes32) { + return keccak256( + bytes( + string.concat( + // Should reflect the definition of the struct `EIP712Domain`. + "EIP712Domain(", + "string name,", + "string version,", + "uint256 chainId,", + "address verifyingContract", + ")" + ) + ) + ); + } + // Ideally, this would be replaced by type(Order).typehash. // Progress tracking for this Solidity feature is here: // https://github.com/ethereum/solidity/issues/14157 @@ -96,6 +122,22 @@ library Eip712 { }); } + function hashStruct(EIP712Domain memory domain) internal pure returns (bytes32) { + // Ideally, this would be replaced by `domain.hashStruct()`. + // Progress tracking for this Solidity feature is here: + // https://github.com/ethereum/solidity/issues/14208 + return keccak256( + // Note: dynamic types are hashed. + abi.encode( + EIP712DOMAIN_TYPE_HASH(), + keccak256(bytes(domain.name)), + keccak256(bytes(domain.version)), + domain.chainId, + domain.verifyingContract + ) + ); + } + function hashStruct(Order memory order) internal pure returns (bytes32) { // Ideally, this would be replaced by `order.hashStruct()`. // Progress tracking for this Solidity feature is here: From c7d4149ac49a7c9708559245984d059d0019f3db Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Fri, 9 Aug 2024 08:14:56 +0000 Subject: [PATCH 02/19] Fix unused import --- test/GPv2Signing/DomainSeparator.t.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/test/GPv2Signing/DomainSeparator.t.sol b/test/GPv2Signing/DomainSeparator.t.sol index 32949567..64c1dab7 100644 --- a/test/GPv2Signing/DomainSeparator.t.sol +++ b/test/GPv2Signing/DomainSeparator.t.sol @@ -5,7 +5,6 @@ import {Helper} from "./Helper.sol"; import {Harness, Helper} from "./Helper.sol"; import {Eip712} from "test/libraries/Eip712.sol"; -import {Order as OrderLib} from "test/libraries/Order.sol"; contract DomainSeparator is Helper { function test_TYPE_HASH_matches_the_EIP_712_order_type_hash() public view { From bc7f235db4dc7d7014a485305c1e6fc3cad59479 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Mon, 12 Aug 2024 06:58:16 +0000 Subject: [PATCH 03/19] Remove duplicated import --- test/GPv2Signing/DomainSeparator.t.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/GPv2Signing/DomainSeparator.t.sol b/test/GPv2Signing/DomainSeparator.t.sol index 64c1dab7..ff359c38 100644 --- a/test/GPv2Signing/DomainSeparator.t.sol +++ b/test/GPv2Signing/DomainSeparator.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: LGPL-3.0-or-later pragma solidity ^0.8; -import {Helper} from "./Helper.sol"; - import {Harness, Helper} from "./Helper.sol"; import {Eip712} from "test/libraries/Eip712.sol"; From 07218923726518edeed494e6efce09ab8c0c107a Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Mon, 12 Aug 2024 07:14:31 +0000 Subject: [PATCH 04/19] chore: migrate GPv2Signing set pre-signature tests to Foundry --- src/ts/sign.ts | 5 --- test/GPv2Signing.test.ts | 47 --------------------- test/GPv2Signing/SetPreSignature.t.sol | 56 ++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 52 deletions(-) create mode 100644 test/GPv2Signing/SetPreSignature.t.sol diff --git a/src/ts/sign.ts b/src/ts/sign.ts index 401270f4..a41e04f2 100644 --- a/src/ts/sign.ts +++ b/src/ts/sign.ts @@ -24,11 +24,6 @@ export const EIP1271_MAGICVALUE = ethers.utils.hexDataSlice( 4, ); -/** - * Marker value indicating a presignature is set. - */ -export const PRE_SIGNED = ethers.utils.id("GPv2Signing.Scheme.PreSign"); - /** * The signing scheme used to sign the order. */ diff --git a/test/GPv2Signing.test.ts b/test/GPv2Signing.test.ts index 1a7c9b4b..87de48f8 100644 --- a/test/GPv2Signing.test.ts +++ b/test/GPv2Signing.test.ts @@ -6,7 +6,6 @@ import { EIP1271_MAGICVALUE, OrderBalance, OrderKind, - PRE_SIGNED, SettlementEncoder, SigningScheme, TypedDataDomain, @@ -14,7 +13,6 @@ import { domain, encodeEip1271SignatureData, hashOrder, - packOrderUidParams, signOrder, } from "../src/ts"; @@ -38,51 +36,6 @@ describe("GPv2Signing", () => { testDomain = domain(chainId, signing.address); }); - describe("setPreSignature", () => { - const [owner, nonOwner] = traders; - const orderUid = packOrderUidParams({ - orderDigest: ethers.constants.HashZero, - owner: owner.address, - validTo: 0xffffffff, - }); - - it("should set the pre-signature", async () => { - await signing.connect(owner).setPreSignature(orderUid, true); - expect(await signing.preSignature(orderUid)).to.equal(PRE_SIGNED); - }); - - it("should unset the pre-signature", async () => { - await signing.connect(owner).setPreSignature(orderUid, true); - await signing.connect(owner).setPreSignature(orderUid, false); - expect(await signing.preSignature(orderUid)).to.equal( - ethers.constants.Zero, - ); - }); - - it("should emit a PreSignature event", async () => { - await expect(signing.connect(owner).setPreSignature(orderUid, true)) - .to.emit(signing, "PreSignature") - .withArgs(owner.address, orderUid, true); - - await expect(signing.connect(owner).setPreSignature(orderUid, false)) - .to.emit(signing, "PreSignature") - .withArgs(owner.address, orderUid, false); - }); - - it("should emit a PreSignature event even if storage doesn't change", async () => { - await signing.connect(owner).setPreSignature(orderUid, true); - await expect(signing.connect(owner).setPreSignature(orderUid, true)) - .to.emit(signing, "PreSignature") - .withArgs(owner.address, orderUid, true); - }); - - it("should revert if the order owner is not the transaction sender", async () => { - await expect( - signing.connect(nonOwner).setPreSignature(orderUid, true), - ).to.be.revertedWith("cannot presign order"); - }); - }); - describe("recoverOrderFromTrade", () => { it("should round-trip encode order data", async () => { // NOTE: Pay extra attention to use all bytes for each field, and that diff --git a/test/GPv2Signing/SetPreSignature.t.sol b/test/GPv2Signing/SetPreSignature.t.sol new file mode 100644 index 00000000..9f422b10 --- /dev/null +++ b/test/GPv2Signing/SetPreSignature.t.sol @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.8; + +import {GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol"; + +import {Helper} from "./Helper.sol"; +import {Order} from "test/libraries/Order.sol"; + +contract SetPreSignature is Helper { + address private immutable owner = makeAddr("GPv2Signing.SetPreSignature owner"); + bytes private orderUid = + Order.computeOrderUid(keccak256("GPv2Signing.SetPreSignature order hash"), owner, type(uint32).max); + + // Same constant as in GPv2Signing.PRE_SIGNED. + uint256 private constant PRE_SIGNED = uint256(keccak256("GPv2Signing.Scheme.PreSign")); + uint256 private constant NOT_SIGNED = 0; + + function test_should_set_the_pre_signature() public { + vm.prank(owner); + executor.setPreSignature(orderUid, true); + assertEq(executor.preSignature(orderUid), PRE_SIGNED); + } + + function test_should_unset_the_pre_signature() public { + vm.prank(owner); + executor.setPreSignature(orderUid, true); + vm.prank(owner); + executor.setPreSignature(orderUid, false); + assertEq(executor.preSignature(orderUid), NOT_SIGNED); + } + + function test_should_emit_a_pre_signature_event() public { + vm.prank(owner); + vm.expectEmit(address(executor)); + emit GPv2Signing.PreSignature(owner, orderUid, true); + executor.setPreSignature(orderUid, true); + + vm.prank(owner); + vm.expectEmit(address(executor)); + emit GPv2Signing.PreSignature(owner, orderUid, false); + executor.setPreSignature(orderUid, false); + } + + function test_should_emit_a_PreSignature_event_even_if_storage_does_not_change() public { + emit GPv2Signing.PreSignature(owner, orderUid, true); + vm.prank(owner); + vm.expectEmit(address(executor)); + emit GPv2Signing.PreSignature(owner, orderUid, true); + executor.setPreSignature(orderUid, true); + } + + function test_reverts_if_the_order_owner_is_not_the_transaction_sender() public { + vm.expectRevert("GPv2: cannot presign order"); + executor.setPreSignature(orderUid, true); + } +} From 56ff59800f58202a230d9cfb453fdb308031a7a8 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Mon, 12 Aug 2024 15:09:46 +0000 Subject: [PATCH 05/19] chore: migrate GPv2Signing recoverOrderFromTrade tests to Foundry --- test/GPv2Signing.test.ts | 111 +------------------ test/GPv2Signing/Helper.sol | 2 + test/GPv2Signing/RecoverOrderFromTrade.t.sol | 76 +++++++++++++ test/libraries/OrderFuzz.sol | 46 ++++++++ 4 files changed, 126 insertions(+), 109 deletions(-) create mode 100644 test/GPv2Signing/RecoverOrderFromTrade.t.sol create mode 100644 test/libraries/OrderFuzz.sol diff --git a/test/GPv2Signing.test.ts b/test/GPv2Signing.test.ts index 87de48f8..b73d22a2 100644 --- a/test/GPv2Signing.test.ts +++ b/test/GPv2Signing.test.ts @@ -4,8 +4,6 @@ import { artifacts, ethers, waffle } from "hardhat"; import { EIP1271_MAGICVALUE, - OrderBalance, - OrderKind, SettlementEncoder, SigningScheme, TypedDataDomain, @@ -16,8 +14,8 @@ import { signOrder, } from "../src/ts"; -import { decodeOrder, encodeOrder } from "./encoding"; -import { fillBytes, fillUint, SAMPLE_ORDER } from "./testHelpers"; +import { encodeOrder } from "./encoding"; +import { SAMPLE_ORDER } from "./testHelpers"; describe("GPv2Signing", () => { const [deployer, ...traders] = waffle.provider.getWallets(); @@ -37,111 +35,6 @@ describe("GPv2Signing", () => { }); describe("recoverOrderFromTrade", () => { - it("should round-trip encode order data", async () => { - // NOTE: Pay extra attention to use all bytes for each field, and that - // they all have different values to make sure the are correctly - // round-tripped. - const order = { - sellToken: fillBytes(20, 0x01), - buyToken: fillBytes(20, 0x02), - receiver: fillBytes(20, 0x03), - sellAmount: fillUint(256, 0x04), - buyAmount: fillUint(256, 0x05), - validTo: fillUint(32, 0x06).toNumber(), - appData: fillBytes(32, 0x07), - feeAmount: fillUint(256, 0x08), - kind: OrderKind.BUY, - partiallyFillable: true, - sellTokenBalance: OrderBalance.EXTERNAL, - buyTokenBalance: OrderBalance.INTERNAL, - }; - const tradeExecution = { - executedAmount: fillUint(256, 0x09), - }; - - const encoder = new SettlementEncoder(testDomain); - await encoder.signEncodeTrade( - order, - traders[0], - SigningScheme.EIP712, - tradeExecution, - ); - - const { data: encodedOrder } = await signing.recoverOrderFromTradeTest( - encoder.tokens, - encoder.trades[0], - ); - expect(decodeOrder(encodedOrder)).to.deep.equal(order); - }); - - it("should compute order unique identifier", async () => { - const encoder = new SettlementEncoder(testDomain); - await encoder.signEncodeTrade( - SAMPLE_ORDER, - traders[0], - SigningScheme.EIP712, - ); - - const { uid: orderUid } = await signing.recoverOrderFromTradeTest( - encoder.tokens, - encoder.trades[0], - ); - expect(orderUid).to.equal( - computeOrderUid(testDomain, SAMPLE_ORDER, traders[0].address), - ); - }); - - it("should recover the owner for all signing schemes", async () => { - const artifact = await artifacts.readArtifact("EIP1271Verifier"); - const verifier = await waffle.deployMockContract(deployer, artifact.abi); - await verifier.mock.isValidSignature.returns(EIP1271_MAGICVALUE); - - const sampleOrderUid = computeOrderUid( - testDomain, - SAMPLE_ORDER, - traders[2].address, - ); - await signing.connect(traders[2]).setPreSignature(sampleOrderUid, true); - - const encoder = new SettlementEncoder(testDomain); - await encoder.signEncodeTrade( - SAMPLE_ORDER, - traders[0], - SigningScheme.EIP712, - ); - await encoder.signEncodeTrade( - SAMPLE_ORDER, - traders[1], - SigningScheme.ETHSIGN, - ); - encoder.encodeTrade(SAMPLE_ORDER, { - scheme: SigningScheme.EIP1271, - data: { - verifier: verifier.address, - signature: "0x", - }, - }); - encoder.encodeTrade(SAMPLE_ORDER, { - scheme: SigningScheme.PRESIGN, - data: traders[2].address, - }); - - const owners = [ - traders[0].address, - traders[1].address, - verifier.address, - traders[2].address, - ]; - - for (const [i, trade] of encoder.trades.entries()) { - const { owner } = await signing.recoverOrderFromTradeTest( - encoder.tokens, - trade, - ); - expect(owner).to.equal(owners[i]); - } - }); - describe("uid uniqueness", () => { it("invalid EVM transaction encoding does not change order hash", async () => { // The variables for an EVM transaction are encoded in multiples of 32 diff --git a/test/GPv2Signing/Helper.sol b/test/GPv2Signing/Helper.sol index 98e07eac..d89c606e 100644 --- a/test/GPv2Signing/Helper.sol +++ b/test/GPv2Signing/Helper.sol @@ -12,8 +12,10 @@ contract Harness is GPv2SigningTestInterface {} contract Helper is Test { Harness internal executor; + bytes32 internal domainSeparator; function setUp() public { executor = new Harness(); + domainSeparator = executor.domainSeparator(); } } diff --git a/test/GPv2Signing/RecoverOrderFromTrade.t.sol b/test/GPv2Signing/RecoverOrderFromTrade.t.sol new file mode 100644 index 00000000..41dd93e9 --- /dev/null +++ b/test/GPv2Signing/RecoverOrderFromTrade.t.sol @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.8; + +import {Vm} from "forge-std/Test.sol"; + +import {EIP1271Verifier, GPv2EIP1271} from "src/contracts/mixins/GPv2Signing.sol"; +import {GPv2Order, GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol"; + +import {Helper} from "./Helper.sol"; +import {Order} from "test/libraries/Order.sol"; + +import {OrderFuzz} from "test/libraries/OrderFuzz.sol"; +import {Sign} from "test/libraries/Sign.sol"; +import {SettlementEncoder} from "test/libraries/encoders/SettlementEncoder.sol"; + +contract RecoverOrderFromTrade is Helper { + using SettlementEncoder for SettlementEncoder.State; + using Sign for EIP1271Verifier; + + Vm.Wallet private trader; + + constructor() { + trader = vm.createWallet("GPv2Signing.RecoverOrderFromTrade: trader"); + } + + function test_should_round_trip_encode_order_data_and_unique_identifier( + OrderFuzz.Params memory params, + uint256 executedAmount + ) public { + GPv2Order.Data memory order = OrderFuzz.order(params); + + SettlementEncoder.State storage encoder = SettlementEncoder.makeSettlementEncoder(); + encoder.signEncodeTrade(vm, trader, order, domainSeparator, GPv2Signing.Scheme.Eip712, executedAmount); + + GPv2Signing.RecoveredOrder memory recovered = + executor.recoverOrderFromTradeTest(encoder.tokens(), encoder.trades[0]); + assertEq(abi.encode(recovered.data), abi.encode(order)); + assertEq(recovered.uid, Order.computeOrderUid(order, domainSeparator, trader.addr)); + } + + function test_should_recover_the_order_for_all_signing_schemes(OrderFuzz.Params memory params) public { + GPv2Order.Data memory order = OrderFuzz.order(params); + + address traderPreSign = makeAddr("trader pre-sign"); + EIP1271Verifier traderEip1271 = EIP1271Verifier(makeAddr("eip1271 verifier")); + Vm.Wallet memory traderEip712 = vm.createWallet("trader eip712"); + Vm.Wallet memory traderEthsign = vm.createWallet("trader ethsign"); + + bytes memory uidPreSign = Order.computeOrderUid(order, domainSeparator, traderPreSign); + vm.prank(traderPreSign); + executor.setPreSignature(uidPreSign, true); + + vm.mockCallRevert(address(traderEip1271), hex"", "unexpected call to mock contract"); + vm.mockCall( + address(traderEip1271), + abi.encodePacked(EIP1271Verifier.isValidSignature.selector), + abi.encode(GPv2EIP1271.MAGICVALUE) + ); + + SettlementEncoder.State storage encoder = SettlementEncoder.makeSettlementEncoder(); + encoder.encodeTrade(order, Sign.preSign(traderPreSign), 0); + encoder.encodeTrade(order, Sign.sign(traderEip1271, hex""), 0); + encoder.signEncodeTrade(vm, traderEip712, order, domainSeparator, GPv2Signing.Scheme.Eip712, 0); + encoder.signEncodeTrade(vm, traderEthsign, order, domainSeparator, GPv2Signing.Scheme.EthSign, 0); + + GPv2Signing.RecoveredOrder memory recovered; + recovered = executor.recoverOrderFromTradeTest(encoder.tokens(), encoder.trades[0]); + assertEq(recovered.owner, traderPreSign); + recovered = executor.recoverOrderFromTradeTest(encoder.tokens(), encoder.trades[1]); + assertEq(recovered.owner, address(traderEip1271)); + recovered = executor.recoverOrderFromTradeTest(encoder.tokens(), encoder.trades[2]); + assertEq(recovered.owner, traderEip712.addr); + recovered = executor.recoverOrderFromTradeTest(encoder.tokens(), encoder.trades[3]); + assertEq(recovered.owner, traderEthsign.addr); + } +} diff --git a/test/libraries/OrderFuzz.sol b/test/libraries/OrderFuzz.sol new file mode 100644 index 00000000..3fa0f4eb --- /dev/null +++ b/test/libraries/OrderFuzz.sol @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.8; + +import {GPv2Order, IERC20} from "src/contracts/libraries/GPv2Order.sol"; +import {GPv2Trade} from "src/contracts/libraries/GPv2Trade.sol"; + +import {Order} from "./Order.sol"; + +library OrderFuzz { + using GPv2Order for GPv2Order.Data; + using GPv2Order for bytes; + using GPv2Trade for uint256; + + struct Params { + address sellToken; + address buyToken; + address receiver; + uint256 sellAmount; + uint256 buyAmount; + uint32 validTo; + bytes32 appData; + uint256 feeAmount; + bytes32 flagsPick; + } + + function order(Params memory params) internal pure returns (GPv2Order.Data memory) { + Order.Flags[] memory allFlags = Order.ALL_FLAGS(); + // `flags` isn't exactly random, but for fuzzing purposes it should be + // more than enough. + Order.Flags memory flags = allFlags[uint256(params.flagsPick) % allFlags.length]; + return GPv2Order.Data({ + sellToken: IERC20(params.sellToken), + buyToken: IERC20(params.buyToken), + receiver: params.receiver, + sellAmount: params.sellAmount, + buyAmount: params.buyAmount, + validTo: params.validTo, + appData: params.appData, + feeAmount: params.feeAmount, + partiallyFillable: flags.partiallyFillable, + kind: flags.kind, + sellTokenBalance: flags.sellTokenBalance, + buyTokenBalance: flags.buyTokenBalance + }); + } +} From 28168278888747872ce1918d54924e6cbdfd8855 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Mon, 12 Aug 2024 15:35:10 +0000 Subject: [PATCH 06/19] Add explanatory comment on domain separator struct --- test/libraries/Eip712.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/libraries/Eip712.sol b/test/libraries/Eip712.sol index 985ace84..82e3f11a 100644 --- a/test/libraries/Eip712.sol +++ b/test/libraries/Eip712.sol @@ -4,6 +4,10 @@ pragma solidity ^0.8; import {GPv2Order} from "src/contracts/libraries/GPv2Order.sol"; library Eip712 { + /// EIP-712 domain information (a.k.a. "domain separator"). The salt + /// parameter is intentionally omitted as there is no need to disambiguate + /// with the available information. More details can be found at: + /// https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator struct EIP712Domain { string name; string version; From 585703de65f95a6ed60bf53bdfee3142969e3339 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Mon, 12 Aug 2024 15:39:34 +0000 Subject: [PATCH 07/19] Reuse existing PRE_SIGNED variable from library --- test/GPv2Signing/SetPreSignature.t.sol | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/GPv2Signing/SetPreSignature.t.sol b/test/GPv2Signing/SetPreSignature.t.sol index 9f422b10..19e6e21b 100644 --- a/test/GPv2Signing/SetPreSignature.t.sol +++ b/test/GPv2Signing/SetPreSignature.t.sol @@ -5,20 +5,17 @@ import {GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol"; import {Helper} from "./Helper.sol"; import {Order} from "test/libraries/Order.sol"; +import {Sign} from "test/libraries/Sign.sol"; contract SetPreSignature is Helper { address private immutable owner = makeAddr("GPv2Signing.SetPreSignature owner"); bytes private orderUid = Order.computeOrderUid(keccak256("GPv2Signing.SetPreSignature order hash"), owner, type(uint32).max); - // Same constant as in GPv2Signing.PRE_SIGNED. - uint256 private constant PRE_SIGNED = uint256(keccak256("GPv2Signing.Scheme.PreSign")); - uint256 private constant NOT_SIGNED = 0; - function test_should_set_the_pre_signature() public { vm.prank(owner); executor.setPreSignature(orderUid, true); - assertEq(executor.preSignature(orderUid), PRE_SIGNED); + assertEq(executor.preSignature(orderUid), Sign.PRE_SIGNED); } function test_should_unset_the_pre_signature() public { @@ -26,7 +23,7 @@ contract SetPreSignature is Helper { executor.setPreSignature(orderUid, true); vm.prank(owner); executor.setPreSignature(orderUid, false); - assertEq(executor.preSignature(orderUid), NOT_SIGNED); + assertEq(executor.preSignature(orderUid), 0); } function test_should_emit_a_pre_signature_event() public { From 84eb7fc3cad25aaf1233e206abfc09c92918f5bc Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Mon, 12 Aug 2024 15:50:31 +0000 Subject: [PATCH 08/19] Fix missing setting of pre-signature at start of test Co-authored-by: mfw78 <53399572+mfw78@users.noreply.github.com> --- test/GPv2Signing/SetPreSignature.t.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/GPv2Signing/SetPreSignature.t.sol b/test/GPv2Signing/SetPreSignature.t.sol index 19e6e21b..e653a1dd 100644 --- a/test/GPv2Signing/SetPreSignature.t.sol +++ b/test/GPv2Signing/SetPreSignature.t.sol @@ -39,7 +39,8 @@ contract SetPreSignature is Helper { } function test_should_emit_a_PreSignature_event_even_if_storage_does_not_change() public { - emit GPv2Signing.PreSignature(owner, orderUid, true); + vm.prank(owner); + executor.setPreSignature(orderUid, true); vm.prank(owner); vm.expectEmit(address(executor)); emit GPv2Signing.PreSignature(owner, orderUid, true); From 6e618dc32a63737497b71618a84f1b1df413e8eb Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 14 Aug 2024 09:24:00 +0000 Subject: [PATCH 09/19] Clean up imports --- test/GPv2Signing/RecoverOrderFromTrade.t.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/GPv2Signing/RecoverOrderFromTrade.t.sol b/test/GPv2Signing/RecoverOrderFromTrade.t.sol index 41dd93e9..96256ebb 100644 --- a/test/GPv2Signing/RecoverOrderFromTrade.t.sol +++ b/test/GPv2Signing/RecoverOrderFromTrade.t.sol @@ -3,12 +3,10 @@ pragma solidity ^0.8; import {Vm} from "forge-std/Test.sol"; -import {EIP1271Verifier, GPv2EIP1271} from "src/contracts/mixins/GPv2Signing.sol"; -import {GPv2Order, GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol"; +import {EIP1271Verifier, GPv2EIP1271, GPv2Order, GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol"; import {Helper} from "./Helper.sol"; import {Order} from "test/libraries/Order.sol"; - import {OrderFuzz} from "test/libraries/OrderFuzz.sol"; import {Sign} from "test/libraries/Sign.sol"; import {SettlementEncoder} from "test/libraries/encoders/SettlementEncoder.sol"; From a8c8b45b50eed945228e30c0c9dc603df4fd2a9f Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 14 Aug 2024 09:28:14 +0000 Subject: [PATCH 10/19] chore: migrate GPv2Signing calldata manipulation tests to Foundry --- test/GPv2Signing.test.ts | 72 +------------------ test/GPv2Signing/CalldataManipulation.t.sol | 79 +++++++++++++++++++++ 2 files changed, 80 insertions(+), 71 deletions(-) create mode 100644 test/GPv2Signing/CalldataManipulation.t.sol diff --git a/test/GPv2Signing.test.ts b/test/GPv2Signing.test.ts index b73d22a2..e977fc5c 100644 --- a/test/GPv2Signing.test.ts +++ b/test/GPv2Signing.test.ts @@ -1,10 +1,9 @@ import { expect } from "chai"; -import { BigNumber, Contract } from "ethers"; +import { Contract } from "ethers"; import { artifacts, ethers, waffle } from "hardhat"; import { EIP1271_MAGICVALUE, - SettlementEncoder, SigningScheme, TypedDataDomain, computeOrderUid, @@ -34,75 +33,6 @@ describe("GPv2Signing", () => { testDomain = domain(chainId, signing.address); }); - describe("recoverOrderFromTrade", () => { - describe("uid uniqueness", () => { - it("invalid EVM transaction encoding does not change order hash", async () => { - // The variables for an EVM transaction are encoded in multiples of 32 - // bytes for all types except `string` and `bytes`. This extra padding - // is usually filled with zeroes by the library that creates the - // transaction. It can however be manually messed with, still producing - // a valid transaction. - // Computing GPv2's orderUid requires copying 32-byte-encoded addresses - // from calldata to memory (buy and sell tokens), which are then hashed - // together with the rest of the order. This copying procedure may keep - // the padding bytes as they are in the (manipulated) calldata, since - // Solidity does not make any guarantees on the padding bits of a - // variable during execution. If these 12 padding bits were not zero - // after copying, then the same order would end up with two different - // uids. This test shows that this is not the case. - - const encoder = new SettlementEncoder(testDomain); - await encoder.signEncodeTrade( - SAMPLE_ORDER, - traders[0], - SigningScheme.EIP712, - ); - - const { uid: orderUid } = await signing.recoverOrderFromTradeTest( - encoder.tokens, - encoder.trades[0], - ); - - const encodedTransactionData = ethers.utils.arrayify( - signing.interface.encodeFunctionData("recoverOrderFromTradeTest", [ - encoder.tokens, - encoder.trades[0], - ]), - ); - - // calldata encoding: - // - 4 bytes: signature - // - 32 bytes: pointer to first input value - // - 32 bytes: pointer to second input value - // - 32 bytes: first input value, array -> token array length - // - 32 bytes: first token address - const encodedNumTokens = BigNumber.from( - encodedTransactionData.slice(4 + 2 * 32, 4 + 3 * 32), - ); - expect(encodedNumTokens).to.equal(2); - const startTokenWord = 4 + 3 * 32; - const encodedFirstToken = encodedTransactionData.slice( - startTokenWord, - startTokenWord + 32, - ); - expect(encodedFirstToken.slice(0, 12).every((byte) => byte === 0)).to.be - .true; - expect(ethers.utils.hexlify(encodedFirstToken.slice(-20))).to.equal( - encoder.tokens[0], - ); - - for (let i = startTokenWord; i < startTokenWord + 12; i++) { - encodedTransactionData[i] = 42; - } - const encodedOutput = await ethers.provider.call({ - data: ethers.utils.hexlify(encodedTransactionData), - to: signing.address, - }); - expect(encodedOutput).to.contain(orderUid.slice(2)); - }); - }); - }); - describe("recoverOrderSigner", () => { it("should recover signing address for all supported ECDSA-based schemes", async () => { for (const scheme of [ diff --git a/test/GPv2Signing/CalldataManipulation.t.sol b/test/GPv2Signing/CalldataManipulation.t.sol new file mode 100644 index 00000000..35c6fd30 --- /dev/null +++ b/test/GPv2Signing/CalldataManipulation.t.sol @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.8; + +import {Vm} from "forge-std/Test.sol"; + +import {GPv2Order, GPv2Signing, IERC20} from "src/contracts/mixins/GPv2Signing.sol"; + +import {GPv2SigningTestInterface, Helper} from "./Helper.sol"; + +import {Bytes} from "test/libraries/Bytes.sol"; +import {OrderFuzz} from "test/libraries/OrderFuzz.sol"; +import {SettlementEncoder} from "test/libraries/encoders/SettlementEncoder.sol"; + +contract CalldataManipulation is Helper { + using SettlementEncoder for SettlementEncoder.State; + using Bytes for bytes; + + Vm.Wallet private trader; + + constructor() { + trader = vm.createWallet("GPv2Signing.RecoverOrderFromTrade: trader"); + } + + function test_invalid_EVM_transaction_encoding_does_not_change_order_hash( + OrderFuzz.Params memory params, + uint256 paddingIndex + ) public { + // The variables for an EVM transaction are encoded in multiples of 32 + // bytes for all types except `string` and `bytes`. This extra padding + // is usually filled with zeroes by the library that creates the + // transaction. It can however be manually messed with. + // Since Solidity v0.8, using nonzero padding should cause the + // transaction to revert. + // Computing GPv2's orderUid requires copying 32-byte-encoded addresses + // from calldata to memory (buy and sell tokens), which are then hashed + // together with the rest of the order. This copying procedure would + // keep the padding bytes as they are in the (manipulated) calldata. + // If these 12 padding bits were not zero after copying, then the same + // order would end up with two different uids. This test shows that this + // is not the case by showing that such calldata manipulation causes the + // transaction to revert. + + GPv2Order.Data memory order = OrderFuzz.order(params); + + SettlementEncoder.State storage encoder = SettlementEncoder.makeSettlementEncoder(); + encoder.signEncodeTrade(vm, trader, order, domainSeparator, GPv2Signing.Scheme.Eip712, 0); + + IERC20[] memory tokens = encoder.tokens(); + bytes memory encodedTransactionData = + abi.encodeCall(GPv2SigningTestInterface.recoverOrderFromTradeTest, (tokens, encoder.trades[0])); + + // calldata encoding: + // - 4 bytes: signature + // - 32 bytes: pointer to first input value + // - 32 bytes: pointer to second input value + // - 32 bytes: first input value, array -> token array length + // - 32 bytes: first token address + uint256 startNumTokenWord = 4 + 2 * 32; + uint256 startFirstTokenWord = startNumTokenWord + 32; + uint256 encodedNumTokens = abi.decode(encodedTransactionData.slice(startNumTokenWord, 32), (uint256)); + require(encodedNumTokens == 2, "invalid test setup; has the transaction encoding changed?"); + bytes memory encodedFirstToken = encodedTransactionData.slice(startFirstTokenWord, 32); + uint256 tokenPaddingSize = 12; + for (uint256 i = 0; i < tokenPaddingSize; i++) { + require(encodedFirstToken[i] == bytes1(0), "invalid test setup; has the transaction encoding changed?"); + } + IERC20 token = IERC20(abi.decode(encodedFirstToken, (address))); + require(token == tokens[0], "invalid test setup; has the transaction encoding changed?"); + // Here we change a single padding byte, this is enough to make the + // transaction revert. + paddingIndex = paddingIndex % tokenPaddingSize; + encodedTransactionData[startFirstTokenWord + paddingIndex] = 0x42; + // Using low-level call because we want to call this function with data + // that is intentionally invalid. + // solhint-disable-next-line avoid-low-level-calls + (bool success,) = address(executor).call(encodedTransactionData); + assertFalse(success); + } +} From ac6bcaef8759229898bdd134f9136037d07b87b0 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 14 Aug 2024 12:18:51 +0000 Subject: [PATCH 11/19] chore: migrate GPv2Signing recoverOrderSigner tests to Foundry --- test/GPv2Signing.test.ts | 301 +--------------------- test/GPv2Signing/Helper.sol | 13 +- test/GPv2Signing/RecoverOrderSigner.t.sol | 208 +++++++++++++++ test/libraries/Sign.sol | 2 + test/src/StateChangingERC1271.sol | 26 -- 5 files changed, 224 insertions(+), 326 deletions(-) create mode 100644 test/GPv2Signing/RecoverOrderSigner.t.sol delete mode 100644 test/src/StateChangingERC1271.sol diff --git a/test/GPv2Signing.test.ts b/test/GPv2Signing.test.ts index e977fc5c..5fa7be76 100644 --- a/test/GPv2Signing.test.ts +++ b/test/GPv2Signing.test.ts @@ -1,299 +1,2 @@ -import { expect } from "chai"; -import { Contract } from "ethers"; -import { artifacts, ethers, waffle } from "hardhat"; - -import { - EIP1271_MAGICVALUE, - SigningScheme, - TypedDataDomain, - computeOrderUid, - domain, - encodeEip1271SignatureData, - hashOrder, - signOrder, -} from "../src/ts"; - -import { encodeOrder } from "./encoding"; -import { SAMPLE_ORDER } from "./testHelpers"; - -describe("GPv2Signing", () => { - const [deployer, ...traders] = waffle.provider.getWallets(); - - let signing: Contract; - let testDomain: TypedDataDomain; - - beforeEach(async () => { - const GPv2Signing = await ethers.getContractFactory( - "GPv2SigningTestInterface", - ); - - signing = await GPv2Signing.deploy(); - - const { chainId } = await ethers.provider.getNetwork(); - testDomain = domain(chainId, signing.address); - }); - - describe("recoverOrderSigner", () => { - it("should recover signing address for all supported ECDSA-based schemes", async () => { - for (const scheme of [ - SigningScheme.EIP712, - SigningScheme.ETHSIGN, - ] as const) { - const { data: signature } = await signOrder( - testDomain, - SAMPLE_ORDER, - traders[0], - scheme, - ); - expect( - await signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - scheme, - signature, - ), - ).to.equal(traders[0].address); - } - }); - - it("should revert for invalid signing schemes", async () => { - await expect( - signing.recoverOrderSignerTest(encodeOrder(SAMPLE_ORDER), 42, "0x"), - ).to.be.reverted; - }); - - it("should revert for malformed ECDSA signatures", async () => { - for (const scheme of [SigningScheme.EIP712, SigningScheme.ETHSIGN]) { - await expect( - signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - scheme, - "0x", - ), - ).to.be.revertedWith("malformed ecdsa signature"); - } - }); - - it("should revert for invalid eip-712 order signatures", async () => { - const { data: signature } = await signOrder( - testDomain, - SAMPLE_ORDER, - traders[0], - SigningScheme.EIP712, - ); - - // NOTE: `v` must be either `27` or `28`, so just set it to something else - // to generate an invalid signature. - const invalidSignature = ethers.utils.arrayify( - ethers.utils.joinSignature(signature), - ); - invalidSignature[64] = 42; - - await expect( - signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - SigningScheme.EIP712, - invalidSignature, - ), - ).to.be.revertedWith("invalid ecdsa signature"); - }); - - it("should revert for invalid ethsign order signatures", async () => { - const { data: signature } = await signOrder( - testDomain, - SAMPLE_ORDER, - traders[0], - SigningScheme.ETHSIGN, - ); - - // NOTE: `v` must be either `27` or `28`, so just set it to something else - // to generate an invalid signature. - const invalidSignature = ethers.utils.arrayify( - ethers.utils.joinSignature(signature), - ); - invalidSignature[64] = 42; - - await expect( - signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - SigningScheme.ETHSIGN, - invalidSignature, - ), - ).to.be.revertedWith("invalid ecdsa signature"); - }); - - it("should verify EIP-1271 contract signatures by returning owner", async () => { - const artifact = await artifacts.readArtifact("EIP1271Verifier"); - const verifier = await waffle.deployMockContract(deployer, artifact.abi); - - const message = hashOrder(testDomain, SAMPLE_ORDER); - const eip1271Signature = "0x031337"; - await verifier.mock.isValidSignature - .withArgs(message, eip1271Signature) - .returns(EIP1271_MAGICVALUE); - - expect( - await signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - SigningScheme.EIP1271, - encodeEip1271SignatureData({ - verifier: verifier.address, - signature: eip1271Signature, - }), - ), - ).to.equal(verifier.address); - }); - - it("should revert on an invalid EIP-1271 signature", async () => { - const message = hashOrder(testDomain, SAMPLE_ORDER); - const eip1271Signature = "0x031337"; - - const artifact = await artifacts.readArtifact("EIP1271Verifier"); - const verifier1 = await waffle.deployMockContract(deployer, artifact.abi); - - await verifier1.mock.isValidSignature - .withArgs(message, eip1271Signature) - .returns("0xbaadc0d3"); - await expect( - signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - SigningScheme.EIP1271, - encodeEip1271SignatureData({ - verifier: verifier1.address, - signature: eip1271Signature, - }), - ), - ).to.be.revertedWith("invalid eip1271 signature"); - }); - - it("should revert with non-standard EIP-1271 verifiers", async () => { - const message = hashOrder(testDomain, SAMPLE_ORDER); - const eip1271Signature = "0x031337"; - - const NON_STANDARD_EIP1271_VERIFIER = [ - "function isValidSignature(bytes32 _hash, bytes memory _signature)", - ]; // no return value - const verifier = await waffle.deployMockContract( - deployer, - NON_STANDARD_EIP1271_VERIFIER, - ); - - await verifier.mock.isValidSignature - .withArgs(message, eip1271Signature) - .returns(); - await expect( - signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - SigningScheme.EIP1271, - encodeEip1271SignatureData({ - verifier: verifier.address, - signature: eip1271Signature, - }), - ), - ).to.be.reverted; - }); - - it("should revert for EIP-1271 signatures from externally owned accounts", async () => { - // Transaction reverted: function call to a non-contract account - await expect( - signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - SigningScheme.EIP1271, - encodeEip1271SignatureData({ - verifier: traders[0].address, - signature: "0x00", - }), - ), - ).to.be.reverted; - }); - - it("should revert if the EIP-1271 verification function changes the state", async () => { - const StateChangingEIP1271 = await ethers.getContractFactory( - "StateChangingEIP1271", - ); - - const evilVerifier = await StateChangingEIP1271.deploy(); - const message = hashOrder(testDomain, SAMPLE_ORDER); - const eip1271Signature = "0x"; - - expect(await evilVerifier.state()).to.equal(ethers.constants.Zero); - await evilVerifier.isValidSignature(message, eip1271Signature); - expect(await evilVerifier.state()).to.equal(ethers.constants.One); - expect( - await evilVerifier.callStatic.isValidSignature( - message, - eip1271Signature, - ), - ).to.equal(EIP1271_MAGICVALUE); - - // Transaction reverted and Hardhat couldn't infer the reason. - await expect( - signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - SigningScheme.EIP1271, - encodeEip1271SignatureData({ - verifier: evilVerifier.address, - signature: eip1271Signature, - }), - ), - ).to.be.reverted; - expect(await evilVerifier.state()).to.equal(ethers.constants.One); - }); - - it("should verify pre-signed order", async () => { - const orderUid = computeOrderUid( - testDomain, - SAMPLE_ORDER, - traders[0].address, - ); - - await signing.connect(traders[0]).setPreSignature(orderUid, true); - expect( - await signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - SigningScheme.PRESIGN, - traders[0].address, - ), - ).to.equal(traders[0].address); - }); - - it("should revert if order doesn't have pre-signature set", async () => { - await expect( - signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - SigningScheme.PRESIGN, - traders[0].address, - ), - ).to.be.revertedWith("order not presigned"); - }); - - it("should revert if pre-signed order is modified", async () => { - await signing - .connect(traders[0]) - .setPreSignature( - computeOrderUid(testDomain, SAMPLE_ORDER, traders[0].address), - true, - ); - - await expect( - signing.recoverOrderSignerTest( - encodeOrder({ - ...SAMPLE_ORDER, - buyAmount: ethers.constants.Zero, - }), - SigningScheme.PRESIGN, - traders[0].address, - ), - ).to.be.revertedWith("order not presigned"); - }); - - it("should revert for malformed pre-sign order UID", async () => { - await expect( - signing.recoverOrderSignerTest( - encodeOrder(SAMPLE_ORDER), - SigningScheme.PRESIGN, - "0x", - ), - ).to.be.revertedWith("malformed presignature"); - }); - }); -}); +// eslint-disable-next-line @typescript-eslint/no-empty-function +describe("GPv2Signing", () => {}); diff --git a/test/GPv2Signing/Helper.sol b/test/GPv2Signing/Helper.sol index d89c606e..295b8880 100644 --- a/test/GPv2Signing/Helper.sol +++ b/test/GPv2Signing/Helper.sol @@ -3,12 +3,23 @@ pragma solidity ^0.8; import {Test} from "forge-std/Test.sol"; +import {GPv2Order} from "src/contracts/mixins/GPv2Signing.sol"; + +import {Sign} from "test/libraries/Sign.sol"; import {GPv2SigningTestInterface} from "test/src/GPv2SigningTestInterface.sol"; // TODO: move the content of `GPv2SigningTestInterface` here once all tests have // been removed. // solhint-disable-next-line no-empty-blocks -contract Harness is GPv2SigningTestInterface {} +contract Harness is GPv2SigningTestInterface { + function recoverOrderSignerTest(GPv2Order.Data memory order, Sign.Signature calldata signature) + public + view + returns (address owner) + { + (, owner) = recoverOrderSigner(order, signature.scheme, signature.data); + } +} contract Helper is Test { Harness internal executor; diff --git a/test/GPv2Signing/RecoverOrderSigner.t.sol b/test/GPv2Signing/RecoverOrderSigner.t.sol new file mode 100644 index 00000000..2c99cd6c --- /dev/null +++ b/test/GPv2Signing/RecoverOrderSigner.t.sol @@ -0,0 +1,208 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.8; + +import {Vm} from "forge-std/Test.sol"; + +import {EIP1271Verifier, GPv2EIP1271, GPv2Order, GPv2Signing, IERC20} from "src/contracts/mixins/GPv2Signing.sol"; + +import {Helper} from "./Helper.sol"; +import {Order} from "test/libraries/Order.sol"; +import {Sign} from "test/libraries/Sign.sol"; + +contract RecoverOrderSigner is Helper { + using GPv2Order for GPv2Order.Data; + + Vm.Wallet private trader; + + constructor() { + trader = vm.createWallet("GPv2Signing.RecoverOrderFromTrade: trader"); + } + + function defaultOrder() internal returns (GPv2Order.Data memory) { + return GPv2Order.Data({ + sellToken: IERC20(makeAddr("GPv2Sign.RecoverOrderSigner: default order sell token")), + buyToken: IERC20(makeAddr("GPv2Sign.RecoverOrderSigner: default order buy token")), + receiver: makeAddr("GPv2Sign.RecoverOrderSigner: default order receiver"), + sellAmount: 42 ether, + buyAmount: 13.37 ether, + validTo: type(uint32).max, + appData: keccak256("GPv2Sign.RecoverOrderSigner: app data"), + feeAmount: 1 ether, + kind: GPv2Order.KIND_SELL, + partiallyFillable: false, + sellTokenBalance: GPv2Order.BALANCE_ERC20, + buyTokenBalance: GPv2Order.BALANCE_ERC20 + }); + } + + function test_should_recover_signing_address_for_all_supported_ECDSA_based_schemes() public { + GPv2Order.Data memory order = defaultOrder(); + + Sign.Signature memory eip712Signature = Sign.sign(vm, trader, order, GPv2Signing.Scheme.Eip712, domainSeparator); + address eip712Owner = executor.recoverOrderSignerTest(order, eip712Signature); + assertEq(eip712Owner, trader.addr); + + Sign.Signature memory ethSignSignature = + Sign.sign(vm, trader, order, GPv2Signing.Scheme.EthSign, domainSeparator); + address ethSignOwner = executor.recoverOrderSignerTest(order, ethSignSignature); + assertEq(ethSignOwner, trader.addr); + } + + function test_reverts_for_invalid_signing_schemes() public { + vm.expectRevert(); + executor.recoverOrderSignerTest( + defaultOrder(), Sign.Signature({scheme: GPv2Signing.Scheme(uint8(42)), data: hex""}) + ); + } + + function test_reverts_for_malformed_ECDSA_signatures() public { + vm.expectRevert("GPv2: malformed ecdsa signature"); + executor.recoverOrderSignerTest( + defaultOrder(), Sign.Signature({scheme: GPv2Signing.Scheme.Eip712, data: hex""}) + ); + vm.expectRevert("GPv2: malformed ecdsa signature"); + executor.recoverOrderSignerTest( + defaultOrder(), Sign.Signature({scheme: GPv2Signing.Scheme.EthSign, data: hex""}) + ); + } + + function test_reverts_for_invalid_eip_712_order_signatures() public { + Sign.Signature memory signature = + Sign.sign(vm, trader, defaultOrder(), GPv2Signing.Scheme.Eip712, domainSeparator); + + // NOTE: `v` must be either `27` or `28`, so just set it to something else + // to generate an invalid signature. + signature.data[64] = bytes1(0x42); + + vm.expectRevert("GPv2: invalid ecdsa signature"); + executor.recoverOrderSignerTest(defaultOrder(), signature); + } + + function test_reverts_for_invalid_ethsign_order_signatures() public { + Sign.Signature memory signature = + Sign.sign(vm, trader, defaultOrder(), GPv2Signing.Scheme.EthSign, domainSeparator); + + // NOTE: `v` must be either `27` or `28`, so just set it to something else + // to generate an invalid signature. + signature.data[64] = bytes1(0x42); + + vm.expectRevert("GPv2: invalid ecdsa signature"); + executor.recoverOrderSignerTest(defaultOrder(), signature); + } + + function test_should_verify_EIP_1271_contract_signatures_by_returning_owner() public { + GPv2Order.Data memory order = defaultOrder(); + bytes32 hash = order.hash(domainSeparator); + + bytes memory eip1271SignatureData = hex"031337"; + EIP1271Verifier verifier = EIP1271Verifier(makeAddr("eip1271 verifier")); + vm.mockCallRevert(address(verifier), hex"", "unexpected call to mock contract"); + vm.mockCall( + address(verifier), + abi.encodeCall(EIP1271Verifier.isValidSignature, (hash, eip1271SignatureData)), + abi.encode(GPv2EIP1271.MAGICVALUE) + ); + + address ethSignOwner = executor.recoverOrderSignerTest(order, Sign.sign(verifier, eip1271SignatureData)); + assertEq(ethSignOwner, address(verifier)); + } + + function test_reverts_on_an_invalid_EIP_1271_signature() public { + GPv2Order.Data memory order = defaultOrder(); + bytes32 hash = order.hash(domainSeparator); + + bytes memory eip1271SignatureData = hex"031337"; + EIP1271Verifier verifier = EIP1271Verifier(makeAddr("eip1271 verifier rejecting signature")); + vm.mockCallRevert(address(verifier), hex"", "unexpected call to mock contract"); + vm.mockCall( + address(verifier), + abi.encodeCall(EIP1271Verifier.isValidSignature, (hash, eip1271SignatureData)), + abi.encode(bytes4(0xbaadc0d3)) + ); + + vm.expectRevert("GPv2: invalid eip1271 signature"); + executor.recoverOrderSignerTest(order, Sign.sign(verifier, eip1271SignatureData)); + } + + function test_reverts_with_non_standard_EIP_1271_verifiers() public { + GPv2Order.Data memory order = defaultOrder(); + bytes32 hash = order.hash(domainSeparator); + + bytes memory eip1271SignatureData = hex"031337"; + EIP1271Verifier verifier = EIP1271Verifier(makeAddr("eip1271 verifier no return data")); + vm.mockCallRevert(address(verifier), hex"", "unexpected call to mock contract"); + vm.mockCall( + address(verifier), abi.encodeCall(EIP1271Verifier.isValidSignature, (hash, eip1271SignatureData)), hex"" + ); + + vm.expectRevert(); + executor.recoverOrderSignerTest(order, Sign.sign(verifier, eip1271SignatureData)); + } + + function test_reverts_for_EIP_1271_signatures_from_externally_owned_accounts() public { + address verifier = makeAddr("externally owned account"); + vm.expectRevert(); + executor.recoverOrderSignerTest(defaultOrder(), Sign.sign(EIP1271Verifier(verifier), hex"00")); + } + + function test_reverts_if_the_EIP_1271_verification_function_changes_the_state() public { + GPv2Order.Data memory order = defaultOrder(); + bytes32 hash = order.hash(domainSeparator); + StateChangingEIP1271 evilVerifier = new StateChangingEIP1271(); + bytes memory eip1271SignatureData = hex""; + + assertEq(evilVerifier.state(), 0); + assertEq(evilVerifier.isValidSignature(hash, eip1271SignatureData), Sign.EIP1271_MAGIC_VALUE); + assertEq(evilVerifier.state(), 1); + vm.expectRevert(); + executor.recoverOrderSignerTest( + defaultOrder(), Sign.sign(EIP1271Verifier(address(evilVerifier)), eip1271SignatureData) + ); + assertEq(evilVerifier.state(), 1); + } + + function test_should_verify_pre_signed_order() public { + GPv2Order.Data memory order = defaultOrder(); + bytes memory orderUid = Order.computeOrderUid(order, domainSeparator, trader.addr); + vm.prank(trader.addr); + executor.setPreSignature(orderUid, true); + address signer = executor.recoverOrderSignerTest(order, Sign.preSign(trader.addr)); + assertEq(signer, trader.addr); + } + + function test_reverts_if_order_does_not_have_pre_signature_set() public { + vm.expectRevert("GPv2: order not presigned"); + executor.recoverOrderSignerTest(defaultOrder(), Sign.preSign(trader.addr)); + } + + function test_should_revert_if_pre_signed_order_is_modified() public { + GPv2Order.Data memory order = defaultOrder(); + bytes memory orderUid = Order.computeOrderUid(order, domainSeparator, trader.addr); + vm.prank(trader.addr); + executor.setPreSignature(orderUid, true); + + order.buyAmount = 0; + vm.expectRevert("GPv2: order not presigned"); + executor.recoverOrderSignerTest(order, Sign.preSign(trader.addr)); + } + + function test_reverts_for_malformed_pre_sign_order_UID() public { + vm.expectRevert("GPv2: malformed presignature"); + executor.recoverOrderSignerTest( + defaultOrder(), Sign.Signature({scheme: GPv2Signing.Scheme.PreSign, data: hex""}) + ); + } +} + +/// @dev This contract implements the standard described in EIP-1271 with the +/// minor change that the verification function changes the state. This is +/// forbidden by the standard specifications. +contract StateChangingEIP1271 { + uint256 public state = 0; + + // solhint-disable-next-line no-unused-vars + function isValidSignature(bytes32, bytes memory) public returns (bytes4) { + state += 1; + return GPv2EIP1271.MAGICVALUE; + } +} diff --git a/test/libraries/Sign.sol b/test/libraries/Sign.sol index 36345e2d..b65dc273 100644 --- a/test/libraries/Sign.sol +++ b/test/libraries/Sign.sol @@ -16,6 +16,8 @@ library Sign { // Copied from GPv2Signing.sol uint256 internal constant PRE_SIGNED = uint256(keccak256("GPv2Signing.Scheme.PreSign")); + // As prescribed by EIP-1271 + bytes4 internal constant EIP1271_MAGIC_VALUE = bytes4(keccak256("isValidSignature(bytes32,bytes)")); /// @dev A struct combining the signing scheme and the scheme's specific-encoded data struct Signature { diff --git a/test/src/StateChangingERC1271.sol b/test/src/StateChangingERC1271.sol deleted file mode 100644 index 3cecafe5..00000000 --- a/test/src/StateChangingERC1271.sol +++ /dev/null @@ -1,26 +0,0 @@ -// SPDX-License-Identifier: LGPL-3.0-or-later -// solhint-disable-next-line compiler-version -pragma solidity >=0.7.6 <0.9.0; - -import "src/contracts/interfaces/GPv2EIP1271.sol"; - -/// @dev This contract implements the standard described in EIP-1271 with the -/// minor change that the verification function changes the state. This is -/// forbidden by the standard specifications. -contract StateChangingEIP1271 { - uint256 public state = 0; - - // solhint-disable-next-line no-unused-vars - function isValidSignature(bytes32 _hash, bytes memory _signature) public returns (bytes4 magicValue) { - state += 1; - magicValue = GPv2EIP1271.MAGICVALUE; - - // The following lines are here to suppress no-unused-var compiler-time - // warnings when compiling the contracts. The warning is forwarded by - // Hardhat from Solc. It is currently not possible to selectively - // ignore Solc warinings: - // - _hash; - _signature; - } -} From e68b367afffc945baee2c07e2cd61b9949b87dda Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 14 Aug 2024 12:31:14 +0000 Subject: [PATCH 12/19] chore: move test contract to GPv2Signing foldedr --- test/GPv2Signing.test.ts | 2 -- test/GPv2Signing/CalldataManipulation.t.sol | 4 +-- test/GPv2Signing/Helper.sol | 17 ++++++++----- test/src/GPv2SigningTestInterface.sol | 27 --------------------- 4 files changed, 13 insertions(+), 37 deletions(-) delete mode 100644 test/GPv2Signing.test.ts delete mode 100644 test/src/GPv2SigningTestInterface.sol diff --git a/test/GPv2Signing.test.ts b/test/GPv2Signing.test.ts deleted file mode 100644 index 5fa7be76..00000000 --- a/test/GPv2Signing.test.ts +++ /dev/null @@ -1,2 +0,0 @@ -// eslint-disable-next-line @typescript-eslint/no-empty-function -describe("GPv2Signing", () => {}); diff --git a/test/GPv2Signing/CalldataManipulation.t.sol b/test/GPv2Signing/CalldataManipulation.t.sol index 35c6fd30..4dd2c087 100644 --- a/test/GPv2Signing/CalldataManipulation.t.sol +++ b/test/GPv2Signing/CalldataManipulation.t.sol @@ -5,7 +5,7 @@ import {Vm} from "forge-std/Test.sol"; import {GPv2Order, GPv2Signing, IERC20} from "src/contracts/mixins/GPv2Signing.sol"; -import {GPv2SigningTestInterface, Helper} from "./Helper.sol"; +import {Harness, Helper} from "./Helper.sol"; import {Bytes} from "test/libraries/Bytes.sol"; import {OrderFuzz} from "test/libraries/OrderFuzz.sol"; @@ -47,7 +47,7 @@ contract CalldataManipulation is Helper { IERC20[] memory tokens = encoder.tokens(); bytes memory encodedTransactionData = - abi.encodeCall(GPv2SigningTestInterface.recoverOrderFromTradeTest, (tokens, encoder.trades[0])); + abi.encodeCall(Harness.recoverOrderFromTradeTest, (tokens, encoder.trades[0])); // calldata encoding: // - 4 bytes: signature diff --git a/test/GPv2Signing/Helper.sol b/test/GPv2Signing/Helper.sol index 295b8880..994f38ed 100644 --- a/test/GPv2Signing/Helper.sol +++ b/test/GPv2Signing/Helper.sol @@ -3,15 +3,20 @@ pragma solidity ^0.8; import {Test} from "forge-std/Test.sol"; -import {GPv2Order} from "src/contracts/mixins/GPv2Signing.sol"; +import {GPv2Order, GPv2Signing, GPv2Trade, IERC20} from "src/contracts/mixins/GPv2Signing.sol"; import {Sign} from "test/libraries/Sign.sol"; -import {GPv2SigningTestInterface} from "test/src/GPv2SigningTestInterface.sol"; -// TODO: move the content of `GPv2SigningTestInterface` here once all tests have -// been removed. -// solhint-disable-next-line no-empty-blocks -contract Harness is GPv2SigningTestInterface { +contract Harness is GPv2Signing { + function recoverOrderFromTradeTest(IERC20[] calldata tokens, GPv2Trade.Data calldata trade) + external + view + returns (GPv2Signing.RecoveredOrder memory recoveredOrder) + { + recoveredOrder = allocateRecoveredOrder(); + recoverOrderFromTrade(recoveredOrder, tokens, trade); + } + function recoverOrderSignerTest(GPv2Order.Data memory order, Sign.Signature calldata signature) public view diff --git a/test/src/GPv2SigningTestInterface.sol b/test/src/GPv2SigningTestInterface.sol deleted file mode 100644 index b0b20270..00000000 --- a/test/src/GPv2SigningTestInterface.sol +++ /dev/null @@ -1,27 +0,0 @@ -// SPDX-License-Identifier: LGPL-3.0-or-later -// solhint-disable-next-line compiler-version -pragma solidity >=0.7.6 <0.9.0; -pragma abicoder v2; - -import "src/contracts/libraries/GPv2Order.sol"; -import "src/contracts/libraries/GPv2Trade.sol"; -import "src/contracts/mixins/GPv2Signing.sol"; - -contract GPv2SigningTestInterface is GPv2Signing { - function recoverOrderFromTradeTest(IERC20[] calldata tokens, GPv2Trade.Data calldata trade) - external - view - returns (RecoveredOrder memory recoveredOrder) - { - recoveredOrder = allocateRecoveredOrder(); - recoverOrderFromTrade(recoveredOrder, tokens, trade); - } - - function recoverOrderSignerTest( - GPv2Order.Data memory order, - GPv2Signing.Scheme signingScheme, - bytes calldata signature - ) external view returns (address owner) { - (, owner) = recoverOrderSigner(order, signingScheme, signature); - } -} From 483e8f2c6bba3dc74c76eb9e4cb3c2570cc5cea7 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 14 Aug 2024 12:36:47 +0000 Subject: [PATCH 13/19] Fix fuzz test case where buy and sell tokens are the same --- test/GPv2Signing/CalldataManipulation.t.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/GPv2Signing/CalldataManipulation.t.sol b/test/GPv2Signing/CalldataManipulation.t.sol index 35c6fd30..2cb9edda 100644 --- a/test/GPv2Signing/CalldataManipulation.t.sol +++ b/test/GPv2Signing/CalldataManipulation.t.sol @@ -58,7 +58,10 @@ contract CalldataManipulation is Helper { uint256 startNumTokenWord = 4 + 2 * 32; uint256 startFirstTokenWord = startNumTokenWord + 32; uint256 encodedNumTokens = abi.decode(encodedTransactionData.slice(startNumTokenWord, 32), (uint256)); - require(encodedNumTokens == 2, "invalid test setup; has the transaction encoding changed?"); + require( + encodedNumTokens == ((order.sellToken == order.buyToken) ? 1 : 2), + "invalid test setup; has the transaction encoding changed?" + ); bytes memory encodedFirstToken = encodedTransactionData.slice(startFirstTokenWord, 32); uint256 tokenPaddingSize = 12; for (uint256 i = 0; i < tokenPaddingSize; i++) { From 8dd66e72b535831ad7e569dea6bc14f3257e4544 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Thu, 15 Aug 2024 12:41:23 +0000 Subject: [PATCH 14/19] Merge fuzzed order library into order library --- test/GPv2Signing/RecoverOrderFromTrade.t.sol | 9 ++-- test/libraries/Order.sol | 34 +++++++++++++++ test/libraries/OrderFuzz.sol | 46 -------------------- 3 files changed, 38 insertions(+), 51 deletions(-) delete mode 100644 test/libraries/OrderFuzz.sol diff --git a/test/GPv2Signing/RecoverOrderFromTrade.t.sol b/test/GPv2Signing/RecoverOrderFromTrade.t.sol index 96256ebb..42cabf14 100644 --- a/test/GPv2Signing/RecoverOrderFromTrade.t.sol +++ b/test/GPv2Signing/RecoverOrderFromTrade.t.sol @@ -7,7 +7,6 @@ import {EIP1271Verifier, GPv2EIP1271, GPv2Order, GPv2Signing} from "src/contract import {Helper} from "./Helper.sol"; import {Order} from "test/libraries/Order.sol"; -import {OrderFuzz} from "test/libraries/OrderFuzz.sol"; import {Sign} from "test/libraries/Sign.sol"; import {SettlementEncoder} from "test/libraries/encoders/SettlementEncoder.sol"; @@ -22,10 +21,10 @@ contract RecoverOrderFromTrade is Helper { } function test_should_round_trip_encode_order_data_and_unique_identifier( - OrderFuzz.Params memory params, + Order.Fuzzed memory params, uint256 executedAmount ) public { - GPv2Order.Data memory order = OrderFuzz.order(params); + GPv2Order.Data memory order = Order.fuzz(params); SettlementEncoder.State storage encoder = SettlementEncoder.makeSettlementEncoder(); encoder.signEncodeTrade(vm, trader, order, domainSeparator, GPv2Signing.Scheme.Eip712, executedAmount); @@ -36,8 +35,8 @@ contract RecoverOrderFromTrade is Helper { assertEq(recovered.uid, Order.computeOrderUid(order, domainSeparator, trader.addr)); } - function test_should_recover_the_order_for_all_signing_schemes(OrderFuzz.Params memory params) public { - GPv2Order.Data memory order = OrderFuzz.order(params); + function test_should_recover_the_order_for_all_signing_schemes(Order.Fuzzed memory params) public { + GPv2Order.Data memory order = Order.fuzz(params); address traderPreSign = makeAddr("trader pre-sign"); EIP1271Verifier traderEip1271 = EIP1271Verifier(makeAddr("eip1271 verifier")); diff --git a/test/libraries/Order.sol b/test/libraries/Order.sol index 1eedf725..393735f8 100644 --- a/test/libraries/Order.sol +++ b/test/libraries/Order.sol @@ -17,6 +17,19 @@ library Order { bool partiallyFillable; } + /// All parameters needed to generated a valid fuzzed order. + struct Fuzzed { + address sellToken; + address buyToken; + address receiver; + uint256 sellAmount; + uint256 buyAmount; + uint32 validTo; + bytes32 appData; + uint256 feeAmount; + bytes32 flagsPick; + } + // I wish I could declare the following as constants and export them as part // of the library. However, "Only constants of value type and byte array // type are implemented." and "Library cannot have non-constant state @@ -145,4 +158,25 @@ library Order { orderUid = new bytes(GPv2Order.UID_LENGTH); orderUid.packOrderUidParams(orderHash, owner, validTo); } + + function fuzz(Fuzzed memory params) internal pure returns (GPv2Order.Data memory) { + Order.Flags[] memory allFlags = Order.ALL_FLAGS(); + // `flags` isn't exactly random, but for fuzzing purposes it should be + // more than enough. + Order.Flags memory flags = allFlags[uint256(params.flagsPick) % allFlags.length]; + return GPv2Order.Data({ + sellToken: IERC20(params.sellToken), + buyToken: IERC20(params.buyToken), + receiver: params.receiver, + sellAmount: params.sellAmount, + buyAmount: params.buyAmount, + validTo: params.validTo, + appData: params.appData, + feeAmount: params.feeAmount, + partiallyFillable: flags.partiallyFillable, + kind: flags.kind, + sellTokenBalance: flags.sellTokenBalance, + buyTokenBalance: flags.buyTokenBalance + }); + } } diff --git a/test/libraries/OrderFuzz.sol b/test/libraries/OrderFuzz.sol deleted file mode 100644 index 3fa0f4eb..00000000 --- a/test/libraries/OrderFuzz.sol +++ /dev/null @@ -1,46 +0,0 @@ -// SPDX-License-Identifier: LGPL-3.0-or-later -pragma solidity ^0.8; - -import {GPv2Order, IERC20} from "src/contracts/libraries/GPv2Order.sol"; -import {GPv2Trade} from "src/contracts/libraries/GPv2Trade.sol"; - -import {Order} from "./Order.sol"; - -library OrderFuzz { - using GPv2Order for GPv2Order.Data; - using GPv2Order for bytes; - using GPv2Trade for uint256; - - struct Params { - address sellToken; - address buyToken; - address receiver; - uint256 sellAmount; - uint256 buyAmount; - uint32 validTo; - bytes32 appData; - uint256 feeAmount; - bytes32 flagsPick; - } - - function order(Params memory params) internal pure returns (GPv2Order.Data memory) { - Order.Flags[] memory allFlags = Order.ALL_FLAGS(); - // `flags` isn't exactly random, but for fuzzing purposes it should be - // more than enough. - Order.Flags memory flags = allFlags[uint256(params.flagsPick) % allFlags.length]; - return GPv2Order.Data({ - sellToken: IERC20(params.sellToken), - buyToken: IERC20(params.buyToken), - receiver: params.receiver, - sellAmount: params.sellAmount, - buyAmount: params.buyAmount, - validTo: params.validTo, - appData: params.appData, - feeAmount: params.feeAmount, - partiallyFillable: flags.partiallyFillable, - kind: flags.kind, - sellTokenBalance: flags.sellTokenBalance, - buyTokenBalance: flags.buyTokenBalance - }); - } -} From 46fcb819f739e520ae91af96d0b8f165351ebdf8 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Thu, 15 Aug 2024 12:49:07 +0000 Subject: [PATCH 15/19] Update fuzz library usage --- test/GPv2Signing/CalldataManipulation.t.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/GPv2Signing/CalldataManipulation.t.sol b/test/GPv2Signing/CalldataManipulation.t.sol index 2cb9edda..c35feebb 100644 --- a/test/GPv2Signing/CalldataManipulation.t.sol +++ b/test/GPv2Signing/CalldataManipulation.t.sol @@ -8,7 +8,7 @@ import {GPv2Order, GPv2Signing, IERC20} from "src/contracts/mixins/GPv2Signing.s import {GPv2SigningTestInterface, Helper} from "./Helper.sol"; import {Bytes} from "test/libraries/Bytes.sol"; -import {OrderFuzz} from "test/libraries/OrderFuzz.sol"; +import {Order} from "test/libraries/Order.sol"; import {SettlementEncoder} from "test/libraries/encoders/SettlementEncoder.sol"; contract CalldataManipulation is Helper { @@ -22,7 +22,7 @@ contract CalldataManipulation is Helper { } function test_invalid_EVM_transaction_encoding_does_not_change_order_hash( - OrderFuzz.Params memory params, + Order.Fuzzed memory params, uint256 paddingIndex ) public { // The variables for an EVM transaction are encoded in multiples of 32 @@ -40,7 +40,7 @@ contract CalldataManipulation is Helper { // is not the case by showing that such calldata manipulation causes the // transaction to revert. - GPv2Order.Data memory order = OrderFuzz.order(params); + GPv2Order.Data memory order = Order.fuzz(params); SettlementEncoder.State storage encoder = SettlementEncoder.makeSettlementEncoder(); encoder.signEncodeTrade(vm, trader, order, domainSeparator, GPv2Signing.Scheme.Eip712, 0); From 16d5d6582b726e3878bd5deb12b9440d5b06ae68 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Thu, 15 Aug 2024 13:35:27 +0000 Subject: [PATCH 16/19] Fix invalid signing scheme test to involve a call --- test/GPv2Signing/Helper.sol | 9 ++++++++- test/GPv2Signing/RecoverOrderSigner.t.sol | 5 ++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/test/GPv2Signing/Helper.sol b/test/GPv2Signing/Helper.sol index 295b8880..03c251cb 100644 --- a/test/GPv2Signing/Helper.sol +++ b/test/GPv2Signing/Helper.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8; import {Test} from "forge-std/Test.sol"; -import {GPv2Order} from "src/contracts/mixins/GPv2Signing.sol"; +import {GPv2Order, GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol"; import {Sign} from "test/libraries/Sign.sol"; import {GPv2SigningTestInterface} from "test/src/GPv2SigningTestInterface.sol"; @@ -19,6 +19,13 @@ contract Harness is GPv2SigningTestInterface { { (, owner) = recoverOrderSigner(order, signature.scheme, signature.data); } + + function uint8ToScheme(uint8 scheme) public pure returns (uint8) { + // Round trip encodes and decodes a uint8 to a Scheme and back. This is + // useful to make sure the code can't use an internally invalid signing + // scheme in its internal operations. + return uint8(GPv2Signing.Scheme(scheme)); + } } contract Helper is Test { diff --git a/test/GPv2Signing/RecoverOrderSigner.t.sol b/test/GPv2Signing/RecoverOrderSigner.t.sol index 2c99cd6c..cda7df2c 100644 --- a/test/GPv2Signing/RecoverOrderSigner.t.sol +++ b/test/GPv2Signing/RecoverOrderSigner.t.sol @@ -49,10 +49,9 @@ contract RecoverOrderSigner is Helper { } function test_reverts_for_invalid_signing_schemes() public { + // panic: failed to convert value into enum type vm.expectRevert(); - executor.recoverOrderSignerTest( - defaultOrder(), Sign.Signature({scheme: GPv2Signing.Scheme(uint8(42)), data: hex""}) - ); + executor.uint8ToScheme(42); } function test_reverts_for_malformed_ECDSA_signatures() public { From 6e97dce1aa401fa194d6d83af1e44e3857ebba1c Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Thu, 15 Aug 2024 13:39:29 +0000 Subject: [PATCH 17/19] Prefer EIP1271Verifier.isValidSignature.selector to static constant --- test/GPv2Signing/RecoverOrderSigner.t.sol | 2 +- test/libraries/Sign.sol | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/test/GPv2Signing/RecoverOrderSigner.t.sol b/test/GPv2Signing/RecoverOrderSigner.t.sol index cda7df2c..b798bae1 100644 --- a/test/GPv2Signing/RecoverOrderSigner.t.sol +++ b/test/GPv2Signing/RecoverOrderSigner.t.sol @@ -151,7 +151,7 @@ contract RecoverOrderSigner is Helper { bytes memory eip1271SignatureData = hex""; assertEq(evilVerifier.state(), 0); - assertEq(evilVerifier.isValidSignature(hash, eip1271SignatureData), Sign.EIP1271_MAGIC_VALUE); + assertEq(evilVerifier.isValidSignature(hash, eip1271SignatureData), EIP1271Verifier.isValidSignature.selector); assertEq(evilVerifier.state(), 1); vm.expectRevert(); executor.recoverOrderSignerTest( diff --git a/test/libraries/Sign.sol b/test/libraries/Sign.sol index aa0b67be..f79c7473 100644 --- a/test/libraries/Sign.sol +++ b/test/libraries/Sign.sol @@ -16,8 +16,6 @@ library Sign { // Copied from GPv2Signing.sol uint256 internal constant PRE_SIGNED = uint256(keccak256("GPv2Signing.Scheme.PreSign")); - // As prescribed by EIP-1271 - bytes4 internal constant EIP1271_MAGIC_VALUE = bytes4(keccak256("isValidSignature(bytes32,bytes)")); /// @dev A struct combining the signing scheme and the scheme's specific-encoded data struct Signature { From a467ea68c7ca1c85ec6f0c2aa788bd4270552c41 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Thu, 15 Aug 2024 15:16:18 +0000 Subject: [PATCH 18/19] Remove test of Solidity enum parsing --- test/GPv2Signing/Helper.sol | 7 ------- test/GPv2Signing/RecoverOrderSigner.t.sol | 6 ------ 2 files changed, 13 deletions(-) diff --git a/test/GPv2Signing/Helper.sol b/test/GPv2Signing/Helper.sol index 03c251cb..088f88e2 100644 --- a/test/GPv2Signing/Helper.sol +++ b/test/GPv2Signing/Helper.sol @@ -19,13 +19,6 @@ contract Harness is GPv2SigningTestInterface { { (, owner) = recoverOrderSigner(order, signature.scheme, signature.data); } - - function uint8ToScheme(uint8 scheme) public pure returns (uint8) { - // Round trip encodes and decodes a uint8 to a Scheme and back. This is - // useful to make sure the code can't use an internally invalid signing - // scheme in its internal operations. - return uint8(GPv2Signing.Scheme(scheme)); - } } contract Helper is Test { diff --git a/test/GPv2Signing/RecoverOrderSigner.t.sol b/test/GPv2Signing/RecoverOrderSigner.t.sol index b798bae1..c5eb5faf 100644 --- a/test/GPv2Signing/RecoverOrderSigner.t.sol +++ b/test/GPv2Signing/RecoverOrderSigner.t.sol @@ -48,12 +48,6 @@ contract RecoverOrderSigner is Helper { assertEq(ethSignOwner, trader.addr); } - function test_reverts_for_invalid_signing_schemes() public { - // panic: failed to convert value into enum type - vm.expectRevert(); - executor.uint8ToScheme(42); - } - function test_reverts_for_malformed_ECDSA_signatures() public { vm.expectRevert("GPv2: malformed ecdsa signature"); executor.recoverOrderSignerTest( From 94bf104d38f65f85be07560b000d45fe19c39f4c Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Thu, 15 Aug 2024 15:19:13 +0000 Subject: [PATCH 19/19] Remove unused import --- test/GPv2Signing/Helper.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/GPv2Signing/Helper.sol b/test/GPv2Signing/Helper.sol index 088f88e2..295b8880 100644 --- a/test/GPv2Signing/Helper.sol +++ b/test/GPv2Signing/Helper.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8; import {Test} from "forge-std/Test.sol"; -import {GPv2Order, GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol"; +import {GPv2Order} from "src/contracts/mixins/GPv2Signing.sol"; import {Sign} from "test/libraries/Sign.sol"; import {GPv2SigningTestInterface} from "test/src/GPv2SigningTestInterface.sol";