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