Skip to content

fix: Execute fill with updated message and recipient #276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 21, 2023
Merged

fix: Execute fill with updated message and recipient #276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9491e95
fix: Execute fill with updated message and recipient
nicholaspai Apr 17, 2023
afd951c
Update SpokePool.Relay.ts
nicholaspai Apr 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions contracts/SpokePool.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -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
);
Comment on lines +1226 to 1240
Copy link
Contributor

@james-a-morris james-a-morris Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should we add the tsconfig setting to require brackets after conditionals?

Copy link
Member Author

@nicholaspai nicholaspai Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know which config we should set for that? I marginally favor requiring brackets but not a big deal

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
);
}
}
Expand Down
21 changes: 16 additions & 5 deletions test/SpokePool.Relay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand Down