diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 3cf7b9ef2..e252c628d 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -10,6 +10,7 @@ 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/MultiCaller.sol"; @@ -722,10 +723,15 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall bytes32 ethSignedMessageHash, bytes memory depositorSignature ) internal view virtual { - // Note: We purposefully do not support EIP-1271 signatures (meaning that multisigs and smart contract wallets - // like Argent are not supported) because of the possibility that a multisig that signed a message on the origin - // chain does not have a parallel on this destination chain. - require(depositor == ECDSA.recover(ethSignedMessageHash, depositorSignature), "invalid signature"); + // Note: + // - We don't need to worry about reentrancy from a contract deployed at the depositor address since the method + // `SignatureChecker.isValidSignatureNow` is a view method. Re-entrancy can happen, but it cannot affect state. + // - EIP-1271 signatures are supported. This means that a signature valid now, may not be valid later and vice-versa. + // - For an EIP-1271 signature to work, the depositor contract address must map to a deployed contract on the destination + // chain that can validate the signature. + // - Regular signatures from an EOA are also supported. + bool isValid = SignatureChecker.isValidSignatureNow(depositor, ethSignedMessageHash, depositorSignature); + require(isValid, "invalid signature"); } function _computeAmountPreFees(uint256 amount, uint64 feesPct) private pure returns (uint256) { diff --git a/contracts/test/MockERC1271.sol b/contracts/test/MockERC1271.sol new file mode 100644 index 000000000..3da5f2440 --- /dev/null +++ b/contracts/test/MockERC1271.sol @@ -0,0 +1,21 @@ +//SPDX-License-Identifier: Unlicense +pragma solidity ^0.8.0; + +import "@openzeppelin/contracts/interfaces/IERC1271.sol"; + +import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import "@openzeppelin/contracts/access/Ownable.sol"; + +/** + * @title MockERC1271 + * @notice Implements mocked ERC1271 contract for testing. + */ +contract MockERC1271 is IERC1271, Ownable { + constructor(address originalOwner) { + transferOwnership(originalOwner); + } + + function isValidSignature(bytes32 hash, bytes memory signature) public view override returns (bytes4 magicValue) { + return ECDSA.recover(hash, signature) == owner() ? this.isValidSignature.selector : bytes4(0); + } +} diff --git a/test/SpokePool.Relay.ts b/test/SpokePool.Relay.ts index 36effad44..345c4aecf 100644 --- a/test/SpokePool.Relay.ts +++ b/test/SpokePool.Relay.ts @@ -3,13 +3,13 @@ import { spokePoolFixture, getRelayHash, modifyRelayHelper } from "./fixtures/Sp import { getFillRelayParams, getFillRelayUpdatedFeeParams } from "./fixtures/SpokePool.Fixture"; import * as consts from "./constants"; -let spokePool: Contract, weth: Contract, erc20: Contract, destErc20: Contract; +let spokePool: Contract, weth: Contract, erc20: Contract, destErc20: Contract, erc1271: Contract; let depositor: SignerWithAddress, recipient: SignerWithAddress, relayer: SignerWithAddress; describe("SpokePool Relayer Logic", async function () { beforeEach(async function () { [depositor, recipient, relayer] = await ethers.getSigners(); - ({ weth, erc20, spokePool, destErc20 } = await spokePoolFixture()); + ({ weth, erc20, spokePool, destErc20, erc1271 } = await spokePoolFixture()); // mint some fresh tokens and deposit ETH for weth for depositor and relayer. await seedWallet(depositor, [erc20], weth, consts.amountToSeedWallets); @@ -175,199 +175,220 @@ describe("SpokePool Relayer Logic", async function () { ).to.be.revertedWith("relay filled"); }); it("Can signal to relayer to use updated fee", async function () { - const spokePoolChainId = await spokePool.chainId(); - const { signature } = await modifyRelayHelper( - consts.modifiedRelayerFeePct, - consts.firstDepositId.toString(), - spokePoolChainId.toString(), - depositor - ); + await testUpdatedFeeSignaling(depositor.address); + }); + it("EIP1271 - Can signal to relayer to use updated fee", async function () { + await testUpdatedFeeSignaling(erc1271.address); + }); + it("Can fill relay with updated fee by including proof of depositor's agreement", async function () { + await testFillRelayWithUpdatedFee(depositor.address); + }); + it("EIP1271 - Can fill relay with updated fee by including proof of depositor's agreement", async function () { + await testFillRelayWithUpdatedFee(erc1271.address); + }); + it("Updating relayer fee signature verification failure cases", async function () { + await testUpdatedFeeSignatureFailCases(depositor.address); + }); + it("EIP1271 - Updating relayer fee signature verification failure cases", async function () { + await testUpdatedFeeSignatureFailCases(erc1271.address); + }); +}); - // Cannot set new relayer fee pct >= 50% - await expect( - spokePool.connect(relayer).speedUpDeposit(depositor.address, toWei("0.5"), consts.firstDepositId, signature) - ).to.be.revertedWith("invalid relayer fee"); +async function testUpdatedFeeSignaling(depositorAddress: string) { + const spokePoolChainId = await spokePool.chainId(); + const { signature } = await modifyRelayHelper( + consts.modifiedRelayerFeePct, + consts.firstDepositId.toString(), + spokePoolChainId.toString(), + depositor + ); - await expect( - spokePool - .connect(relayer) - .speedUpDeposit(depositor.address, consts.modifiedRelayerFeePct, consts.firstDepositId, signature) - ) - .to.emit(spokePool, "RequestedSpeedUpDeposit") - .withArgs(consts.modifiedRelayerFeePct, consts.firstDepositId, depositor.address, signature); + // Cannot set new relayer fee pct >= 50% + await expect( + spokePool.connect(relayer).speedUpDeposit(depositorAddress, toWei("0.5"), consts.firstDepositId, signature) + ).to.be.revertedWith("invalid relayer fee"); - // Reverts if any param passed to function is changed. - await expect( - spokePool - .connect(relayer) - .speedUpDeposit(relayer.address, consts.modifiedRelayerFeePct, consts.firstDepositId, signature) - ).to.be.reverted; - await expect(spokePool.connect(relayer).speedUpDeposit(depositor.address, "0", consts.firstDepositId, signature)).to - .be.reverted; - await expect( - spokePool - .connect(relayer) - .speedUpDeposit(depositor.address, consts.modifiedRelayerFeePct, consts.firstDepositId + 1, signature) - ).to.be.reverted; - await expect( - spokePool - .connect(relayer) - .speedUpDeposit(depositor.address, consts.modifiedRelayerFeePct, consts.firstDepositId, "0xrandombytes") - ).to.be.reverted; - const { signature: incorrectOriginChainIdSignature } = await modifyRelayHelper( - consts.modifiedRelayerFeePct, - consts.firstDepositId.toString(), - consts.originChainId.toString(), - depositor - ); - await expect( - spokePool - .connect(relayer) - .speedUpDeposit( - depositor.address, - consts.modifiedRelayerFeePct, - consts.firstDepositId, - incorrectOriginChainIdSignature - ) - ).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 } = getRelayHash( - depositor.address, - recipient.address, - consts.firstDepositId, - consts.originChainId, - consts.destinationChainId, - destErc20.address - ); - const { signature } = await modifyRelayHelper( - consts.modifiedRelayerFeePct, - relayData.depositId, - relayData.originChainId, - depositor - ); - await expect( - spokePool - .connect(relayer) - .fillRelayWithUpdatedFee( - ...getFillRelayUpdatedFeeParams(relayData, consts.amountToRelay, consts.modifiedRelayerFeePct, signature) - ) - ) - .to.emit(spokePool, "FilledRelay") - .withArgs( - relayData.amount, - consts.amountToRelayPreModifiedFees, - consts.amountToRelayPreModifiedFees, - consts.repaymentChainId, - toBN(relayData.originChainId), - toBN(relayData.destinationChainId), - relayData.relayerFeePct, - consts.modifiedRelayerFeePct, // Applied relayer fee % should be diff from original fee %. - relayData.realizedLpFeePct, - toBN(relayData.depositId), - relayData.destinationToken, - relayer.address, - relayData.depositor, - relayData.recipient, - false - ); + await expect( + spokePool + .connect(relayer) + .speedUpDeposit(depositorAddress, consts.modifiedRelayerFeePct, consts.firstDepositId, signature) + ) + .to.emit(spokePool, "RequestedSpeedUpDeposit") + .withArgs(consts.modifiedRelayerFeePct, consts.firstDepositId, depositorAddress, signature); - // The collateral should have transferred from relayer to recipient. - expect(await destErc20.balanceOf(relayer.address)).to.equal(consts.amountToSeedWallets.sub(consts.amountToRelay)); - expect(await destErc20.balanceOf(recipient.address)).to.equal(consts.amountToRelay); + // Reverts if any param passed to function is changed. + await expect( + spokePool + .connect(relayer) + .speedUpDeposit(relayer.address, consts.modifiedRelayerFeePct, consts.firstDepositId, signature) + ).to.be.reverted; + await expect(spokePool.connect(relayer).speedUpDeposit(depositorAddress, "0", consts.firstDepositId, signature)).to.be + .reverted; + await expect( + spokePool + .connect(relayer) + .speedUpDeposit(depositorAddress, consts.modifiedRelayerFeePct, consts.firstDepositId + 1, signature) + ).to.be.reverted; + await expect( + spokePool + .connect(relayer) + .speedUpDeposit(depositorAddress, consts.modifiedRelayerFeePct, consts.firstDepositId, "0xrandombytes") + ).to.be.reverted; + const { signature: incorrectOriginChainIdSignature } = await modifyRelayHelper( + consts.modifiedRelayerFeePct, + consts.firstDepositId.toString(), + consts.originChainId.toString(), + depositor + ); + await expect( + spokePool + .connect(relayer) + .speedUpDeposit( + depositorAddress, + consts.modifiedRelayerFeePct, + consts.firstDepositId, + incorrectOriginChainIdSignature + ) + ).to.be.reverted; +} - // Fill amount should be be set taking into account modified fees. - expect(await spokePool.relayFills(relayHash)).to.equal(consts.amountToRelayPreModifiedFees); - }); - it("Updating relayer fee signature verification failure cases", async function () { - const { relayData } = getRelayHash( - depositor.address, - recipient.address, - consts.firstDepositId, - consts.originChainId, - consts.destinationChainId, - destErc20.address +async function testFillRelayWithUpdatedFee(depositorAddress: string) { + // 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 } = getRelayHash( + depositorAddress, + recipient.address, + consts.firstDepositId, + consts.originChainId, + consts.destinationChainId, + destErc20.address + ); + const { signature } = await modifyRelayHelper( + consts.modifiedRelayerFeePct, + relayData.depositId, + relayData.originChainId, + depositor + ); + await expect( + spokePool + .connect(relayer) + .fillRelayWithUpdatedFee( + ...getFillRelayUpdatedFeeParams(relayData, consts.amountToRelay, consts.modifiedRelayerFeePct, signature) + ) + ) + .to.emit(spokePool, "FilledRelay") + .withArgs( + relayData.amount, + consts.amountToRelayPreModifiedFees, + consts.amountToRelayPreModifiedFees, + consts.repaymentChainId, + toBN(relayData.originChainId), + toBN(relayData.destinationChainId), + relayData.relayerFeePct, + consts.modifiedRelayerFeePct, // Applied relayer fee % should be diff from original fee %. + relayData.realizedLpFeePct, + toBN(relayData.depositId), + relayData.destinationToken, + relayer.address, + relayData.depositor, + relayData.recipient, + false ); - // Message hash doesn't contain the modified fee passed as a function param. - const { signature: incorrectFeeSignature } = await modifyRelayHelper( - consts.incorrectModifiedRelayerFeePct, - relayData.depositId, - relayData.originChainId, - depositor - ); - await expect( - spokePool - .connect(relayer) - .fillRelayWithUpdatedFee( - ...getFillRelayUpdatedFeeParams( - relayData, - consts.amountToRelay, - consts.modifiedRelayerFeePct, - incorrectFeeSignature - ) + // The collateral should have transferred from relayer to recipient. + expect(await destErc20.balanceOf(relayer.address)).to.equal(consts.amountToSeedWallets.sub(consts.amountToRelay)); + expect(await destErc20.balanceOf(recipient.address)).to.equal(consts.amountToRelay); + + // Fill amount should be be set taking into account modified fees. + expect(await spokePool.relayFills(relayHash)).to.equal(consts.amountToRelayPreModifiedFees); +} + +async function testUpdatedFeeSignatureFailCases(depositorAddress: string) { + const { relayData } = getRelayHash( + depositorAddress, + recipient.address, + consts.firstDepositId, + consts.originChainId, + consts.destinationChainId, + destErc20.address + ); + + // Message hash doesn't contain the modified fee passed as a function param. + const { signature: incorrectFeeSignature } = await modifyRelayHelper( + consts.incorrectModifiedRelayerFeePct, + relayData.depositId, + relayData.originChainId, + depositor + ); + await expect( + spokePool + .connect(relayer) + .fillRelayWithUpdatedFee( + ...getFillRelayUpdatedFeeParams( + relayData, + consts.amountToRelay, + consts.modifiedRelayerFeePct, + incorrectFeeSignature ) - ).to.be.revertedWith("invalid signature"); + ) + ).to.be.revertedWith("invalid signature"); - // Relay data depositID and originChainID don't match data included in relay hash - const { signature: incorrectDepositIdSignature } = await modifyRelayHelper( - consts.modifiedRelayerFeePct, - relayData.depositId + "1", - relayData.originChainId, - depositor - ); - await expect( - spokePool - .connect(relayer) - .fillRelayWithUpdatedFee( - ...getFillRelayUpdatedFeeParams( - relayData, - consts.amountToRelay, - consts.modifiedRelayerFeePct, - incorrectDepositIdSignature - ) + // Relay data depositID and originChainID don't match data included in relay hash + const { signature: incorrectDepositIdSignature } = await modifyRelayHelper( + consts.modifiedRelayerFeePct, + relayData.depositId + "1", + relayData.originChainId, + depositor + ); + await expect( + spokePool + .connect(relayer) + .fillRelayWithUpdatedFee( + ...getFillRelayUpdatedFeeParams( + relayData, + consts.amountToRelay, + consts.modifiedRelayerFeePct, + incorrectDepositIdSignature ) - ).to.be.revertedWith("invalid signature"); - const { signature: incorrectChainIdSignature } = await modifyRelayHelper( - consts.modifiedRelayerFeePct, - relayData.depositId, - relayData.originChainId + "1", - depositor - ); - await expect( - spokePool - .connect(relayer) - .fillRelayWithUpdatedFee( - ...getFillRelayUpdatedFeeParams( - relayData, - consts.amountToRelay, - consts.modifiedRelayerFeePct, - incorrectChainIdSignature - ) + ) + ).to.be.revertedWith("invalid signature"); + const { signature: incorrectChainIdSignature } = await modifyRelayHelper( + consts.modifiedRelayerFeePct, + relayData.depositId, + relayData.originChainId + "1", + depositor + ); + await expect( + spokePool + .connect(relayer) + .fillRelayWithUpdatedFee( + ...getFillRelayUpdatedFeeParams( + relayData, + consts.amountToRelay, + consts.modifiedRelayerFeePct, + incorrectChainIdSignature ) - ).to.be.revertedWith("invalid signature"); + ) + ).to.be.revertedWith("invalid signature"); - // Message hash must be signed by depositor passed in function params. - const { signature: incorrectSignerSignature } = await modifyRelayHelper( - consts.modifiedRelayerFeePct, - relayData.depositId, - relayData.originChainId, - relayer - ); - await expect( - spokePool - .connect(relayer) - .fillRelayWithUpdatedFee( - ...getFillRelayUpdatedFeeParams( - relayData, - consts.amountToRelay, - consts.modifiedRelayerFeePct, - incorrectSignerSignature - ) + // Message hash must be signed by depositor passed in function params. + const { signature: incorrectSignerSignature } = await modifyRelayHelper( + consts.modifiedRelayerFeePct, + relayData.depositId, + relayData.originChainId, + relayer + ); + await expect( + spokePool + .connect(relayer) + .fillRelayWithUpdatedFee( + ...getFillRelayUpdatedFeeParams( + relayData, + consts.amountToRelay, + consts.modifiedRelayerFeePct, + incorrectSignerSignature ) - ).to.be.revertedWith("invalid signature"); - }); -}); + ) + ).to.be.revertedWith("invalid signature"); +} diff --git a/test/fixtures/SpokePool.Fixture.ts b/test/fixtures/SpokePool.Fixture.ts index dc6494cee..b127c0f9a 100644 --- a/test/fixtures/SpokePool.Fixture.ts +++ b/test/fixtures/SpokePool.Fixture.ts @@ -14,6 +14,7 @@ export async function deploySpokePool(ethers: any): Promise<{ spokePool: Contract; unwhitelistedErc20: Contract; destErc20: Contract; + erc1271: Contract; }> { const [deployerWallet, crossChainAdmin, hubPool] = await ethers.getSigners(); // Useful contracts. @@ -38,7 +39,10 @@ export async function deploySpokePool(ethers: any): Promise<{ ).deploy(0, crossChainAdmin.address, hubPool.address, weth.address, timer.address); await spokePool.setChainId(consts.destinationChainId); - return { timer, weth, erc20, spokePool, unwhitelistedErc20, destErc20 }; + // ERC1271 + const erc1271 = await (await getContractFactory("MockERC1271", deployerWallet)).deploy(deployerWallet.address); + + return { timer, weth, erc20, spokePool, unwhitelistedErc20, destErc20, erc1271 }; } export interface DepositRoute {