From 9491e95db1fbd8852141569a2c198a45e394a970 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 17 Apr 2023 17:16:04 -0400 Subject: [PATCH 1/2] fix: Execute fill with updated message and recipient When sped up, updated message and recipient are not used --- contracts/SpokePool.sol | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index c12bf4d8c..cac520df0 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -1212,7 +1212,7 @@ abstract contract SpokePool is // net movement of funds. // Note: this is important because it means that relayers can intentionally self-relay in a capital efficient // way (no need to have funds on the destination). - if (msg.sender == relayData.recipient) return fillAmountPreFees; + if (msg.sender == relayExecution.updatedRecipient) return fillAmountPreFees; // If relay token is wrappedNativeToken then unwrap and send native token. if (relayData.destinationToken == address(wrappedNativeToken)) { @@ -1223,24 +1223,28 @@ abstract contract SpokePool is // need to unwrap it to native token before sending to the user. if (!relayExecution.slowFill) IERC20Upgradeable(relayData.destinationToken).safeTransferFrom(msg.sender, address(this), amountToSend); - _unwrapwrappedNativeTokenTo(payable(relayData.recipient), amountToSend); + _unwrapwrappedNativeTokenTo(payable(relayExecution.updatedRecipient), amountToSend); // Else, this is a normal ERC20 token. Send to recipient. } else { // Note: Similar to note above, send token directly from the contract to the user in the slow relay case. if (!relayExecution.slowFill) IERC20Upgradeable(relayData.destinationToken).safeTransferFrom( msg.sender, - relayData.recipient, + relayExecution.updatedRecipient, + amountToSend + ); + else + IERC20Upgradeable(relayData.destinationToken).safeTransfer( + relayExecution.updatedRecipient, amountToSend ); - else IERC20Upgradeable(relayData.destinationToken).safeTransfer(relayData.recipient, amountToSend); } - if (relayData.recipient.isContract() && relayData.message.length > 0) { - AcrossMessageHandler(relayData.recipient).handleAcrossMessage( + if (relayExecution.updatedRecipient.isContract() && relayExecution.updatedMessage.length > 0) { + AcrossMessageHandler(relayExecution.updatedRecipient).handleAcrossMessage( relayData.destinationToken, amountToSend, - relayData.message + relayExecution.updatedMessage ); } } From afd951c4079b9d096ee4d8b047438892532065e7 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 17 Apr 2023 17:33:45 -0400 Subject: [PATCH 2/2] Update SpokePool.Relay.ts --- test/SpokePool.Relay.ts | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/test/SpokePool.Relay.ts b/test/SpokePool.Relay.ts index c83083815..4414b9ab9 100644 --- a/test/SpokePool.Relay.ts +++ b/test/SpokePool.Relay.ts @@ -584,20 +584,23 @@ async function testUpdatedFeeSignaling(depositorAddress: string) { } async function testfillRelayWithUpdatedDeposit(depositorAddress: string) { + const acrossMessageHandler = await createFake("AcrossMessageHandlerMock"); + const updatedRecipient = acrossMessageHandler.address; + const updatedMessage = "0x1234"; + // 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, + depositor.address, consts.firstDepositId, consts.originChainId, consts.destinationChainId, destErc20.address ); - - const updatedRecipient = depositor.address; - const updatedMessage = "0x1234"; + expect(relayData.message).to.not.equal(updatedMessage); + expect(relayData.recipient).to.not.equal(updatedRecipient); const { signature } = await modifyRelayHelper( consts.modifiedRelayerFeePct, @@ -646,6 +649,14 @@ async function testfillRelayWithUpdatedDeposit(depositorAddress: string) { ] ); + // Check that updated message and recipient are used with executed fill: + const amountActuallySent = await destErc20.balanceOf(acrossMessageHandler.address); + expect(acrossMessageHandler.handleAcrossMessage).to.have.been.calledOnceWith( + relayData.destinationToken, + amountActuallySent, + updatedMessage + ); + // The collateral should have transferred from relayer to recipient. const relayerBalance = await destErc20.balanceOf(relayer.address); const expectedRelayerBalance = consts.amountToSeedWallets.sub(consts.amountToRelay); @@ -654,7 +665,7 @@ async function testfillRelayWithUpdatedDeposit(depositorAddress: string) { // of rounding errors with the modified fees. The unmodified fees result in clean numbers but the modified fee does not. expect(relayerBalance.gte(expectedRelayerBalance.sub(1)) || relayerBalance.lte(expectedRelayerBalance.add(1))).to.be .true; - const recipientBalance = await destErc20.balanceOf(recipient.address); + const recipientBalance = amountActuallySent; const expectedRecipientBalance = consts.amountToRelay; expect(recipientBalance.gte(expectedRecipientBalance.sub(1)) || recipientBalance.lte(expectedRecipientBalance.add(1))) .to.be.true;