From 1d1628cba20c80759503cebcd4954eefb1fc33fd Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Thu, 15 Aug 2024 13:48:11 +0000 Subject: [PATCH 1/2] chore: migrate GPv2Signing recoverOrderFromTrade tests to Foundry (#204) ## Description See title. This also introduces a small library to make it easier to create a test that uses fuzzed orders. Fuzzed tests are used here because the exact bytes used in the tests are important, and fuzzing allows us to check if changing some bytes affects the test output. Previously, the sample order in the test was built by filling each byte with a nonzero value. I also merged the tests "should round-trip encode order data" and "should compute order unique identifier" because it was just one line more in the first test, and now that we do fuzzing it's more efficient to do both in the same test rather than running independent fuzzes on both. Here the signing and encoding libraries proved to be very nice to use! ## Test Plan CI. ## Related Issues #120 --------- Co-authored-by: mfw78 <53399572+mfw78@users.noreply.github.com> --- test/GPv2Signing.test.ts | 111 +------------------ test/GPv2Signing/Helper.sol | 2 + test/GPv2Signing/RecoverOrderFromTrade.t.sol | 73 ++++++++++++ test/libraries/Order.sol | 34 ++++++ 4 files changed, 111 insertions(+), 109 deletions(-) create mode 100644 test/GPv2Signing/RecoverOrderFromTrade.t.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..42cabf14 --- /dev/null +++ b/test/GPv2Signing/RecoverOrderFromTrade.t.sol @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.8; + +import {Vm} from "forge-std/Test.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 {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( + Order.Fuzzed memory params, + uint256 executedAmount + ) public { + GPv2Order.Data memory order = Order.fuzz(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(Order.Fuzzed memory params) public { + GPv2Order.Data memory order = Order.fuzz(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/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 + }); + } } From 2536fa33c4882c030c10bb6fd43b9a0d77c2b7c1 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Thu, 15 Aug 2024 13:51:28 +0000 Subject: [PATCH 2/2] chore: migrate GPv2Signing calldata manipulation tests to Foundry (#206) ## Description There is only one test involved, and the logic of it has changed in the upgrade process. The key difference is that we use Solidity version 0.8 in the tests, while the previous test used Solidity 0.7. The relevant part here is that contract calls now validate the padding in the calldata, making it impossible to successfully call `recoverOrderFromTrade` with manipulated calldata. Curiously, this should have been the case since Solidity ~0.5, however it wasn't properly enforced until Solidity 0.8 (see ethereum/solidity repo [here](https://github.com/ethereum/solidity/blob/5647d9910af304446073a380c49ff455dbe4c7ea/test/externalTests/gp2.sh#L98-L101) and discussion in the PR where this was introduced). ## Test Plan CI. ## Related Issues #120 --------- Co-authored-by: mfw78 <53399572+mfw78@users.noreply.github.com> --- test/GPv2Signing.test.ts | 72 +----------------- test/GPv2Signing/CalldataManipulation.t.sol | 82 +++++++++++++++++++++ 2 files changed, 83 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..c35feebb --- /dev/null +++ b/test/GPv2Signing/CalldataManipulation.t.sol @@ -0,0 +1,82 @@ +// 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 {Order} from "test/libraries/Order.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( + Order.Fuzzed 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 = Order.fuzz(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 == ((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++) { + 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); + } +}