diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 9c553097b..d9a4fc2a1 100644 --- a/contracts/SpokePool.sol +++ b/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"; @@ -192,6 +195,10 @@ abstract contract SpokePool is Testable, Lockable, MultiCaller { numberOfDeposits += 1; } + /************************************** + * RELAYER FUNCTIONS * + **************************************/ + function fillRelay( address depositor, address recipient, @@ -204,12 +211,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,59 +226,75 @@ abstract contract SpokePool is Testable, Lockable, MultiCaller { }); bytes32 relayHash = _getRelayHash(relayData); - // Check that 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(relayFills[relayHash] < totalRelayAmount, "relay filled"); - - // Stores the equivalent amount to be sent by the relayer before fees have been taken out. - uint256 fillAmountPreFees = 0; + _fillRelay(relayHash, relayData, relayerFeePct, maxTokensToSend, repaymentChain); + } - // Adding brackets "stack too deep" solidity error. - if (maxTokensToSend > 0) { - fillAmountPreFees = _computeAmountPreFees(maxTokensToSend, (realizedLpFeePct + relayerFeePct)); - // 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)); - } - 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); - // Else, this is a normal ERC20 token. Send to recipient. - } else IERC20(destinationToken).safeTransferFrom(msg.sender, recipient, amountToSend); + // 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. + function fillRelayWithUpdatedFee( + address depositor, + address recipient, + address destinationToken, + uint64 realizedLpFeePct, + uint64 relayerFeePct, + uint64 newRelayerFeePct, + uint64 depositId, + uint256 originChainId, + uint256 totalRelayAmount, + uint256 maxTokensToSend, + uint256 repaymentChain, + bytes memory depositorSignature + ) + 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", newRelayerFeePct, depositId, originChainId) + ); + + // 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. + // If the depositor signed a message with a different updated fee (or any other param included in the + // above keccak156 hash), then this will revert. + bytes32 ethSignedMessageHash = ECDSA.toEthSignedMessageHash(expectedDepositorMessageHash); + + // Note: no need to worry about reentrancy from contract deployed at `depositor` address since + // `SignatureChecker.isValidSignatureNow` is a non state-modifying STATICCALL: + // - https://github.com/OpenZeppelin/openzeppelin-contracts/blob/63b466901fb015538913f811c5112a2775042177/contracts/utils/cryptography/SignatureChecker.sol#L35 + // - https://github.com/ethereum/EIPs/pull/214 + require( + SignatureChecker.isValidSignatureNow(depositor, ethSignedMessageHash, depositorSignature), + "invalid signature" + ); } - 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 - ); + // 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, 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. - // Once this method is executed and a distribution root is stored in this contract, then `distributeRelayerRefund` - // can be called to execute each leaf in the root. - function _initializeRelayerRefund(bytes32 relayerRepaymentDistributionProof) internal { - uint256 relayerRefundId = relayerRefunds.length; - relayerRefunds.push().distributionRoot = relayerRepaymentDistributionProof; - emit InitializedRelayerRefund(relayerRefundId, relayerRepaymentDistributionProof); - } + /************************************** + * DATA WORKER FUNCTIONS * + **************************************/ // Call this method to execute a leaf within the `distributionRoot` stored on this contract. Caller must include a // valid `inclusionProof` to verify that the leaf is contained within the root. The `relayerRefundId` is the index @@ -323,6 +340,93 @@ abstract contract SpokePool is Testable, Lockable, MultiCaller { } } + // 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. + // Once this method is executed and a distribution root is stored in this contract, then `distributeRelayerRefund` + // can be called to execute each leaf in the root. + function _initializeRelayerRefund(bytes32 relayerRepaymentDistributionProof) internal { + uint256 relayerRefundId = relayerRefunds.length; + relayerRefunds.push().distributionRoot = relayerRepaymentDistributionProof; + emit InitializedRelayerRefund(relayerRefundId, relayerRepaymentDistributionProof); + } + + 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. Note that 0.5e18 (i.e. 50%) + // fees are just magic numbers. The important point is to prevent the total fee from being 100%, otherwise + // computing the amount pre fees runs into divide-by-0 issues. + require(updatableRelayerFeePct < 0.5e18 && relayData.realizedLpFeePct < 0.5e18, "invalid fees"); + + // Check that 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(relayFills[relayHash] < relayData.relayAmount, "relay filled"); + + // Stores the equivalent amount to be sent by the relayer before fees have been taken out. + uint256 fillAmountPreFees = 0; + + // Adding brackets "stack too deep" solidity error. + if (maxTokensToSend > 0) { + fillAmountPreFees = _computeAmountPreFees( + maxTokensToSend, + (relayData.realizedLpFeePct + updatableRelayerFeePct) + ); + // 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 (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 (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(relayData.destinationToken).safeTransferFrom(msg.sender, relayData.recipient, amountToSend); + } + + // Needed to resolve stack too deep compile-time error + _emitFillRelay( + FillRelayEventData(relayHash, relayData, updatableRelayerFeePct, fillAmountPreFees, repaymentChain) + ); + } + + 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 {} } diff --git a/test/SpokePool.Fixture.ts b/test/SpokePool.Fixture.ts index 74ace12de..062160c95 100644 --- a/test/SpokePool.Fixture.ts +++ b/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,28 @@ 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 }> { + 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, + }; +} diff --git a/test/SpokePool.Relay.ts b/test/SpokePool.Relay.ts index ec0e04542..5d481a4e1 100644 --- a/test/SpokePool.Relay.ts +++ b/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,11 @@ import { repaymentChainId, firstDepositId, oneHundredPct, + modifiedRelayerFeePct, + incorrectModifiedRelayerFeePct, + amountToRelayPreModifiedFees, + realizedLpFeePct, + depositRelayerFeePct, } from "./constants"; let spokePool: Contract, weth: Contract, erc20: Contract, destErc20: Contract; @@ -143,24 +148,6 @@ describe("SpokePool Relayer Logic", async function () { }); it("General failure cases", async function () { // Fees set too high. - await expect( - spokePool - .connect(relayer) - .fillRelay( - ...getRelayHash( - depositor.address, - recipient.address, - firstDepositId, - originChainId, - destErc20.address, - amountToDeposit.toString(), - toWei("0.51").toString(), - toWei("0.5").toString() - ).relayDataValues, - amountToRelay, - repaymentChainId - ) - ).to.be.revertedWith("invalid fees"); await expect( spokePool .connect(relayer) @@ -173,7 +160,7 @@ describe("SpokePool Relayer Logic", async function () { destErc20.address, amountToDeposit.toString(), toWei("0.5").toString(), - toWei("0.51").toString() + depositRelayerFeePct.toString() ).relayDataValues, amountToRelay, repaymentChainId @@ -190,7 +177,7 @@ describe("SpokePool Relayer Logic", async function () { originChainId, destErc20.address, amountToDeposit.toString(), - toWei("0.5").toString(), + realizedLpFeePct.toString(), toWei("0.5").toString() ).relayDataValues, amountToRelay, @@ -216,4 +203,111 @@ describe("SpokePool Relayer Logic", async function () { ) ).to.be.revertedWith("relay filled"); }); + 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 { signature } = await modifyRelayHelper( + modifiedRelayerFeePct, + relayData.depositId, + relayData.originChainId, + depositor + ); + // Note: modifiedRelayFeePct is inserted in-place into middle of the same params passed to fillRelay + relayDataValues.splice(5, 0, modifiedRelayerFeePct.toString()); + await expect( + spokePool.connect(relayer).fillRelayWithUpdatedFee(...relayDataValues, amountToRelay, repaymentChainId, signature) + ) + .to.emit(spokePool, "FilledRelay") + .withArgs( + relayHash, + relayData.relayAmount, + amountToRelayPreModifiedFees, + amountToRelayPreModifiedFees, + repaymentChainId, + relayData.originChainId, + relayData.depositId, + modifiedRelayerFeePct, // 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 + ); + // Note: modifiedRelayFeePct is inserted in-place into middle of the same params passed to fillRelay + relayDataValues.splice(5, 0, modifiedRelayerFeePct.toString()); + + // Message hash doesn't contain the modified fee passed as a function param. + const { signature: incorrectFeeSignature } = await modifyRelayHelper( + incorrectModifiedRelayerFeePct, + relayData.depositId, + relayData.originChainId, + depositor + ); + await expect( + spokePool + .connect(relayer) + .fillRelayWithUpdatedFee(...relayDataValues, amountToRelay, repaymentChainId, incorrectFeeSignature) + ).to.be.revertedWith("invalid signature"); + + // Relay data depositID and originChainID don't match data included in relay hash + const { signature: incorrectDepositIdSignature } = await modifyRelayHelper( + incorrectModifiedRelayerFeePct, + relayData.depositId + "1", + relayData.originChainId, + depositor + ); + await expect( + spokePool + .connect(relayer) + .fillRelayWithUpdatedFee(...relayDataValues, amountToRelay, repaymentChainId, incorrectDepositIdSignature) + ).to.be.revertedWith("invalid signature"); + const { signature: incorrectChainIdSignature } = await modifyRelayHelper( + incorrectModifiedRelayerFeePct, + relayData.depositId, + relayData.originChainId + "1", + depositor + ); + await expect( + spokePool + .connect(relayer) + .fillRelayWithUpdatedFee(...relayDataValues, amountToRelay, repaymentChainId, incorrectChainIdSignature) + ).to.be.revertedWith("invalid signature"); + + // Message hash must be signed by depositor passed in function params. + const { signature: incorrectSignerSignature } = await modifyRelayHelper( + modifiedRelayerFeePct, + relayData.depositId, + relayData.originChainId, + relayer + ); + await expect( + spokePool + .connect(relayer) + .fillRelayWithUpdatedFee(...relayDataValues, amountToRelay, repaymentChainId, incorrectSignerSignature) + ).to.be.revertedWith("invalid signature"); + }); }); diff --git a/test/constants.ts b/test/constants.ts index 575d0b757..a5ffd96ce 100644 --- a/test/constants.ts +++ b/test/constants.ts @@ -10,14 +10,22 @@ export const amountToRelay = toWei("25"); export const depositRelayerFeePct = toWei("0.1"); +export const modifiedRelayerFeePct = toBN(depositRelayerFeePct).add(toBN(toWei("0.1"))); + +export const incorrectModifiedRelayerFeePct = toBN(modifiedRelayerFeePct).add(toBN(toWei("0.01"))); + 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;