feat(spoke-pool): Allow relayer and depositor to agree to update relay fee

contracts/SpokePool.sol

@@ -4,6 +4,9 @@ pragma solidity ^0.8.0;
 import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import "@openzeppelin/contracts/utils/Address.sol";
+import "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol";
+import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
+
 import "@uma/core/contracts/common/implementation/Testable.sol";
 import "@uma/core/contracts/common/implementation/Lockable.sol";
 import "@uma/core/contracts/common/implementation/MultiCaller.sol";
@@ -204,12 +207,6 @@ abstract contract SpokePool is Testable, Lockable, MultiCaller {
         uint256 maxTokensToSend,
         uint256 repaymentChain
     ) public {
-        // We limit the relay fees to prevent the user spending all their funds on fees.
-        require(
-            relayerFeePct <= 0.5e18 && realizedLpFeePct <= 0.5e18 && (relayerFeePct + realizedLpFeePct) < 1e18,
-            "invalid fees"
-        );
-
         // Each relay attempt is mapped to the hash of data uniquely identifying it, which includes the deposit data
         // such as the origin chain ID and the deposit ID, and the data in a relay attempt such as who the recipient
         // is, which chain and currency the recipient wants to receive funds on, and the relay fees.
@@ -225,49 +222,130 @@ abstract contract SpokePool is Testable, Lockable, MultiCaller {
         });
         bytes32 relayHash = _getRelayHash(relayData);
 
+        _fillRelay(relayHash, relayData, relayerFeePct, maxTokensToSend, repaymentChain);
+    }
+
+    function _fillRelay(
+        bytes32 relayHash,
+        RelayData memory relayData,
+        uint64 updatableRelayerFeePct,
+        uint256 maxTokensToSend,
+        uint256 repaymentChain
+    ) internal {
+        // We limit the relay fees to prevent the user spending all their funds on fees.
+        require(
+            updatableRelayerFeePct <= 0.5e18 &&
+                relayData.realizedLpFeePct <= 0.5e18 &&
+                (updatableRelayerFeePct + relayData.realizedLpFeePct) < 1e18,
+            "invalid fees"
+        );
+
         // Check that the caller is filling a non zero amount of the relay and the relay has not already been completely
         // filled. Note that the `relays` mapping will point to the amount filled so far for a particular `relayHash`,
         // so this will start at 0 and increment with each fill.
-        require(maxTokensToSend > 0 && relayFills[relayHash] < totalRelayAmount, "Cannot send 0, or relay filled");
+        require(maxTokensToSend > 0 && relayFills[relayHash] < relayData.relayAmount, "Cannot send 0, or relay filled");
 
         // Stores the equivalent amount to be sent by the relayer before fees have been taken out.
-        uint256 fillAmountPreFees = _computeAmountPreFees(maxTokensToSend, (realizedLpFeePct + relayerFeePct));
+        uint256 fillAmountPreFees = _computeAmountPreFees(
+            maxTokensToSend,
+            (relayData.realizedLpFeePct + updatableRelayerFeePct)
+        );
 
         // Adding brackets "stack too deep" solidity error.
         {
             // If user's specified max amount to send is greater than the amount of the relay remaining pre-fees,
             // we'll pull exactly enough tokens to complete the relay.
             uint256 amountToSend = maxTokensToSend;
-            if (totalRelayAmount - relayFills[relayHash] < fillAmountPreFees) {
-                fillAmountPreFees = totalRelayAmount - relayFills[relayHash];
-                amountToSend = _computeAmountPostFees(fillAmountPreFees, (realizedLpFeePct + relayerFeePct));
+            if (relayData.relayAmount - relayFills[relayHash] < fillAmountPreFees) {
+                fillAmountPreFees = relayData.relayAmount - relayFills[relayHash];
+                amountToSend = _computeAmountPostFees(
+                    fillAmountPreFees,
+                    relayData.realizedLpFeePct + updatableRelayerFeePct
+                );
             }
             relayFills[relayHash] += fillAmountPreFees;
             // If relay token is weth then unwrap and send eth.
-            if (destinationToken == address(weth)) {
-                IERC20(destinationToken).safeTransferFrom(msg.sender, address(this), amountToSend);
-                _unwrapWETHTo(payable(recipient), amountToSend);
+            if (relayData.destinationToken == address(weth)) {
+                IERC20(relayData.destinationToken).safeTransferFrom(msg.sender, address(this), amountToSend);
+                _unwrapWETHTo(payable(relayData.recipient), amountToSend);
                 // Else, this is a normal ERC20 token. Send to recipient.
-            } else IERC20(destinationToken).safeTransferFrom(msg.sender, recipient, amountToSend);
+            } else IERC20(relayData.destinationToken).safeTransferFrom(msg.sender, relayData.recipient, amountToSend);
         }
 
-        emit FilledRelay(
-            relayHash,
-            relayData.relayAmount,
-            relayFills[relayHash],
-            fillAmountPreFees,
-            repaymentChain,
-            relayData.originChainId,
-            relayData.depositId,
-            relayData.relayerFeePct,
-            relayData.realizedLpFeePct,
-            relayData.destinationToken,
-            msg.sender,
-            relayData.depositor,
-            relayData.recipient
+        // Needed to resolve stack too deep compile-time error
+        _emitFillRelay(
+            FillRelayEventData(relayHash, relayData, updatableRelayerFeePct, fillAmountPreFees, repaymentChain)
         );
     }
 
+    // We overload `fillRelay` logic to allow the relayer to optionally pass in an updated `relayerFeePct` and a signature
+    // proving that the depositor agreed to the updated fee.
+    struct UpdatedRelayerFeeData {
+        // Note: This struct is used to make sure that the following overloaded `fillRelay` does not create a stack
+        // too deep compile-time error.
+        uint64 newRelayerFeePct;
+        bytes32 depositorMessageHash;
+        bytes depositorSignature;
+    }
+
+    function fillRelayUpdatedFee(
+        address depositor,
+        address recipient,
+        address destinationToken,
+        uint64 realizedLpFeePct,
+        uint64 relayerFeePct,
+        uint64 depositId,
+        uint256 originChainId,
+        uint256 totalRelayAmount,
+        uint256 maxTokensToSend,
+        uint256 repaymentChain,
+        UpdatedRelayerFeeData memory updatedRelayerFeeData // Note: Ideally we don't pass in any complex objects to
+    )
+        public
+        // public methods but I couldn't figure out a way to pass this in without encounering a stack too deep error.
+        nonReentrant
+    {
+        // Grouping the signature validation logic into brackets to address stack too deep error.
+        {
+            // Depositor should have signed a hash of the relayer fee % to update to and information uniquely identifying
+            // the deposit to relay. This ensures that this signature cannot be re-used for other deposits. The version
+            // string is included as a precaution in case this contract is upgraded.
+            // Note: we use encode instead of encodePacked because it is more secure, more in the "warning" section
+            // here: https://docs.soliditylang.org/en/v0.8.11/abi-spec.html#non-standard-packed-mode
+            bytes32 expectedDepositorMessageHash = keccak256(
+                abi.encode("ACROSS-V2-FEE-1.0", updatedRelayerFeeData.newRelayerFeePct, depositId, originChainId)
+            );
+            require(expectedDepositorMessageHash == updatedRelayerFeeData.depositorMessageHash, "incorrect new fee");
+
+            // Check the hash corresponding to the https://eth.wiki/json-rpc/API#eth_sign[`eth_sign`]
+            // JSON-RPC method as part of EIP-191. We use OZ's signature checker library with adds support for
+            // EIP-1271 which can verify messages signed by smart contract wallets like Argent and Gnosis safes.
+            bytes32 ethSignedMessageHash = ECDSA.toEthSignedMessageHash(expectedDepositorMessageHash);
+            require(
+                SignatureChecker.isValidSignatureNow(
+                    depositor,
+                    ethSignedMessageHash,
+                    updatedRelayerFeeData.depositorSignature
+                ),
+                "invalid signature"
+            );
+        }
+
+        // Now follow the default `fillRelay` flow with the updated fee and the original relay hash.
+        RelayData memory relayData = RelayData({
+            depositor: depositor,
+            recipient: recipient,
+            destinationToken: destinationToken,
+            realizedLpFeePct: realizedLpFeePct,
+            relayerFeePct: relayerFeePct,
+            depositId: depositId,
+            originChainId: originChainId,
+            relayAmount: totalRelayAmount
+        });
+        bytes32 relayHash = _getRelayHash(relayData);
+        _fillRelay(relayHash, relayData, updatedRelayerFeeData.newRelayerFeePct, maxTokensToSend, repaymentChain);
+    }
+
     // This internal method should be called by an external "initializeRelayerRefund" function that validates the
     // cross domain sender is the HubPool. This validation step differs for each L2, which is why the implementation
     // specifics are left to the implementor of this abstract contract.
@@ -323,6 +401,32 @@ abstract contract SpokePool is Testable, Lockable, MultiCaller {
         }
     }
 
+    struct FillRelayEventData {
+        bytes32 relayHash;
+        RelayData relayData;
+        uint64 updatableRelayerFeePct;
+        uint256 fillAmountPreFees;
+        uint256 repaymentChain;
+    }
+
+    function _emitFillRelay(FillRelayEventData memory eventData) internal {
+        emit FilledRelay(
+            eventData.relayHash,
+            eventData.relayData.relayAmount,
+            relayFills[eventData.relayHash],
+            eventData.fillAmountPreFees,
+            eventData.repaymentChain,
+            eventData.relayData.originChainId,
+            eventData.relayData.depositId,
+            eventData.updatableRelayerFeePct,
+            eventData.relayData.realizedLpFeePct,
+            eventData.relayData.destinationToken,
+            msg.sender,
+            eventData.relayData.depositor,
+            eventData.relayData.recipient
+        );
+    }
+
     // Added to enable the this contract to receive ETH. Used when unwrapping Weth.
     receive() external payable {}
 }

test/SpokePool.Fixture.ts

@@ -1,5 +1,5 @@
 import { TokenRolesEnum } from "@uma/common";
-import { Contract, utils } from "ethers";
+import { BigNumber, Contract, utils } from "ethers";
 import { getContractFactory, SignerWithAddress } from "./utils";
 import {
   destinationChainId,
@@ -10,7 +10,7 @@ import {
 } from "./constants";
 import hre from "hardhat";
 
-const { defaultAbiCoder, keccak256 } = utils;
+const { defaultAbiCoder, keccak256, arrayify } = utils;
 
 export const spokePoolFixture = hre.deployments.createFixture(async ({ ethers }) => {
   const [deployerWallet] = await ethers.getSigners();
@@ -114,3 +114,33 @@ export function getRelayHash(
     relayDataValues,
   };
 }
+
+export interface UpdatedRelayerFeeData {
+  newRelayerFeePct: string;
+  depositorMessageHash: string;
+  depositorSignature: string;
+}
+export async function modifyRelayHelper(
+  modifiedRelayerFeePct: BigNumber,
+  depositId: string,
+  originChainId: string,
+  depositor: SignerWithAddress
+): Promise<{ messageHash: string; signature: string; updatedRelayerFeeData: UpdatedRelayerFeeData }> {
+  const messageHash = keccak256(
+    defaultAbiCoder.encode(
+      ["string", "uint64", "uint64", "uint256"],
+      ["ACROSS-V2-FEE-1.0", modifiedRelayerFeePct, depositId, originChainId]
+    )
+  );
+  const signature = await depositor.signMessage(arrayify(messageHash));
+
+  return {
+    messageHash,
+    signature,
+    updatedRelayerFeeData: {
+      newRelayerFeePct: modifiedRelayerFeePct.toString(),
+      depositorMessageHash: messageHash,
+      depositorSignature: signature,
+    },
+  };
+}

test/SpokePool.Relay.ts

@@ -1,8 +1,8 @@
 import { expect } from "chai";
-import { Contract } from "ethers";
 import { ethers } from "hardhat";
+import { Contract } from "ethers";
 import { SignerWithAddress, seedWallet, toWei, toBN } from "./utils";
-import { spokePoolFixture, enableRoutes, getRelayHash } from "./SpokePool.Fixture";
+import { spokePoolFixture, enableRoutes, getRelayHash, modifyRelayHelper } from "./SpokePool.Fixture";
 import {
   amountToSeedWallets,
   amountToDeposit,
@@ -13,6 +13,8 @@ import {
   repaymentChainId,
   firstDepositId,
   oneHundredPct,
+  modifiedRelayerFeePct,
+  amountToRelayPreModifiedFees,
 } from "./constants";
 
 let spokePool: Contract, weth: Contract, erc20: Contract, destErc20: Contract;
@@ -228,4 +230,86 @@ describe("SpokePool Relayer Logic", async function () {
         )
     ).to.be.reverted;
   });
+  it("Can fill relay with updated fee by including proof of depositor's agreement", async function () {
+    // The relay should succeed just like before with the same amount of tokens pulled from the relayer's wallet,
+    // however the filled amount should have increased since the proportion of the relay filled would increase with a
+    // higher fee.
+    const { relayHash, relayData, relayDataValues } = getRelayHash(
+      depositor.address,
+      recipient.address,
+      firstDepositId,
+      originChainId,
+      destErc20.address
+    );
+    const { updatedRelayerFeeData } = await modifyRelayHelper(
+      modifiedRelayerFeePct,
+      relayData.depositId,
+      relayData.originChainId,
+      depositor
+    );
+    await expect(
+      spokePool
+        .connect(relayer)
+        .fillRelayUpdatedFee(...relayDataValues, amountToRelay, repaymentChainId, updatedRelayerFeeData)
+    )
+      .to.emit(spokePool, "FilledRelay")
+      .withArgs(
+        relayHash,
+        relayData.relayAmount,
+        amountToRelayPreModifiedFees,
+        amountToRelayPreModifiedFees,
+        repaymentChainId,
+        relayData.originChainId,
+        relayData.depositId,
+        updatedRelayerFeeData.newRelayerFeePct, // The relayer fee % emitted in event should change
+        relayData.realizedLpFeePct,
+        relayData.destinationToken,
+        relayer.address,
+        relayData.depositor,
+        relayData.recipient
+      );
+
+    // The collateral should have transferred from relayer to recipient.
+    expect(await destErc20.balanceOf(relayer.address)).to.equal(amountToSeedWallets.sub(amountToRelay));
+    expect(await destErc20.balanceOf(recipient.address)).to.equal(amountToRelay);
+
+    // Fill amount should be be set taking into account modified fees.
+    expect(await spokePool.relayFills(relayHash)).to.equal(amountToRelayPreModifiedFees);
+  });
+  it("Updating relayer fee signature verification failure cases", async function () {
+    const { relayDataValues, relayData } = getRelayHash(
+      depositor.address,
+      recipient.address,
+      firstDepositId,
+      originChainId,
+      destErc20.address
+    );
+    const { updatedRelayerFeeData } = await modifyRelayHelper(
+      modifiedRelayerFeePct,
+      relayData.depositId,
+      relayData.originChainId,
+      depositor
+    );
+
+    // Message hash doesn't contain the modified fee passed as a function param:
+    await expect(
+      spokePool.connect(relayer).fillRelayUpdatedFee(...relayDataValues, amountToRelay, repaymentChainId, {
+        ...updatedRelayerFeeData,
+        newRelayerFeePct: "0", // This fee passed as a function param doesn't match the one signed by depositor.
+      })
+    ).to.be.revertedWith("incorrect new fee");
+
+    // Message hash must be signed by depositor passed in function params.
+    const { updatedRelayerFeeData: incorrectSigner } = await modifyRelayHelper(
+      modifiedRelayerFeePct,
+      relayData.depositId,
+      relayData.originChainId,
+      relayer
+    );
+    await expect(
+      spokePool
+        .connect(relayer)
+        .fillRelayUpdatedFee(...relayDataValues, amountToRelay, repaymentChainId, incorrectSigner)
+    ).to.be.revertedWith("invalid signature");
+  });
 });

test/constants.ts

@@ -10,14 +10,20 @@ export const amountToRelay = toWei("25");
 
 export const depositRelayerFeePct = toWei("0.1");
 
+export const modifiedRelayerFeePct = toBN(depositRelayerFeePct).add(toBN(toWei("0.1")));
+
 export const realizedLpFeePct = toWei("0.1");
 
 export const oneHundredPct = toWei("1");
 
 export const totalPostFeesPct = toBN(oneHundredPct).sub(toBN(depositRelayerFeePct).add(realizedLpFeePct));
 
+export const totalPostModifiedFeesPct = toBN(oneHundredPct).sub(toBN(modifiedRelayerFeePct).add(realizedLpFeePct));
+
 export const amountToRelayPreFees = toBN(amountToRelay).mul(toBN(oneHundredPct)).div(totalPostFeesPct);
 
+export const amountToRelayPreModifiedFees = toBN(amountToRelay).mul(toBN(oneHundredPct)).div(totalPostModifiedFeesPct);
+
 export const destinationChainId = 1337;
 
 export const originChainId = 666;