Skip to content

fix: Prevent partial fills with messages #278

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

Closed
wants to merge 4 commits into from
Closed

fix: Prevent partial fills with messages #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e8bfb77
Update SpokePool.sol
nicholaspai Apr 21, 2023
9ef36c3
Block partial fills with messages
nicholaspai Apr 20, 2023
59a9225
Update SpokePool.SlowRelay.ts
nicholaspai Apr 21, 2023
2165c92
Update SpokePool.SlowRelay.ts
nicholaspai Apr 21, 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
7 changes: 7 additions & 0 deletions contracts/SpokePool.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1193,6 +1193,13 @@ abstract contract SpokePool is
"invalid repayment chain"
);

// If this is a partial fill, require that the fill has no message. This prevents situations in which
// a message is sent multiple times for multiple partial fills.
require(
relayExecution.relay.amount == fillAmountPreFees || relayExecution.updatedMessage.length == 0,
"invalid partial fill message"
Copy link
Contributor

@pxrl pxrl Apr 21, 2023

Choose a reason for hiding this comment

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

Suggested change
"invalid partial fill message"
"invalid partial fill (message)"

Copy link
Member Author

@nicholaspai nicholaspai Apr 22, 2023

Choose a reason for hiding this comment

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

you think this improves the error message a bit? I'm not convinced but curious what you were thinking?

Copy link
Contributor

@pxrl pxrl Apr 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

you think this improves the error message a bit? I'm not convinced but curious what you were thinking?

Yeah..even though I know exactly what the issue is, I have trouble parsing "invalid partial fill message" in my head. How about this?

Suggested change
"invalid partial fill message"
"invalid message on partial fill"

Alternatively:

Suggested change
"invalid partial fill message"
"non-zero message on partial fill"

);

// Update fill counter.
_updateCountFromFill(
relayFills[relayExecution.relayHash],
Expand Down
24 changes: 15 additions & 9 deletions test/SpokePool.Relay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,18 +305,24 @@ describe("SpokePool Relayer Logic", async function () {
"0x1234"
);

// Partial fills fail with a message:
await expect(
spokePool
.connect(relayer)
.fillRelay(...getFillRelayParams(relayData, consts.amountToRelay, consts.destinationChainId))
).to.be.revertedWith("invalid partial fill message");
await spokePool
.connect(relayer)
.fillRelay(...getFillRelayParams(relayData, consts.amountToRelay, consts.destinationChainId));
.fillRelay(...getFillRelayParams(relayData, relayData.amount, consts.destinationChainId));

expect(acrossMessageHandler.handleAcrossMessage).to.have.been.calledOnceWith(
weth.address,
consts.amountToRelay,
consts.amountToDepositPostFees,
"0x1234"
);

// The collateral should have not unwrapped to ETH and then transferred to recipient.
expect(await weth.balanceOf(acrossMessageHandler.address)).to.equal(consts.amountToRelay);
expect(await weth.balanceOf(acrossMessageHandler.address)).to.equal(consts.amountToDepositPostFees);
});
it("Self-relay transfers no tokens", async function () {
const largeRelayAmount = consts.amountToSeedWallets.mul(100);
Expand Down Expand Up @@ -616,7 +622,7 @@ async function testfillRelayWithUpdatedDeposit(depositorAddress: string) {
.fillRelayWithUpdatedDeposit(
...getFillRelayUpdatedFeeParams(
relayData,
consts.amountToRelay,
relayData.amount,
consts.modifiedRelayerFeePct,
signature,
consts.destinationChainId,
Expand All @@ -628,8 +634,8 @@ async function testfillRelayWithUpdatedDeposit(depositorAddress: string) {
.to.emit(spokePool, "FilledRelay")
.withArgs(
relayData.amount,
consts.amountToRelayPreModifiedFees,
consts.amountToRelayPreModifiedFees,
relayData.amount,
relayData.amount,
consts.destinationChainId,
toBN(relayData.originChainId),
toBN(relayData.destinationChainId),
Expand Down Expand Up @@ -659,19 +665,19 @@ async function testfillRelayWithUpdatedDeposit(depositorAddress: string) {

// The collateral should have transferred from relayer to recipient.
const relayerBalance = await destErc20.balanceOf(relayer.address);
const expectedRelayerBalance = consts.amountToSeedWallets.sub(consts.amountToRelay);
const expectedRelayerBalance = consts.amountToSeedWallets.sub(consts.amountToDepositPostFees);

// Note: We need to add an error bound of 1 wei to the expected balance because of the possibility
// 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 = amountActuallySent;
const expectedRecipientBalance = consts.amountToRelay;
const expectedRecipientBalance = consts.amountToDepositPostFees;
expect(recipientBalance.gte(expectedRecipientBalance.sub(1)) || recipientBalance.lte(expectedRecipientBalance.add(1)))
.to.be.true;

// Fill amount should be be set taking into account modified fees.
expect(await spokePool.relayFills(relayHash)).to.equal(consts.amountToRelayPreModifiedFees);
expect(await spokePool.relayFills(relayHash)).to.equal(relayData.amount);
}

async function testUpdatedFeeSignatureFailCases(depositorAddress: string) {
Expand Down
91 changes: 58 additions & 33 deletions test/SpokePool.SlowRelay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,36 +79,46 @@ describe("SpokePool Slow Relay Logic", async function () {
}

// ERC20
const erc20LeafRelayData = {
depositor: depositor.address,
recipient: recipient.address,
destinationToken: destErc20.address,
amount: consts.amountToRelay,
originChainId: consts.originChainId.toString(),
destinationChainId: consts.destinationChainId.toString(),
realizedLpFeePct: consts.realizedLpFeePct,
relayerFeePct: consts.depositRelayerFeePct,
depositId: consts.firstDepositId.toString(),
message: erc20Message,
};
slowFills.push({
relayData: {
depositor: depositor.address,
recipient: recipient.address,
destinationToken: destErc20.address,
amount: consts.amountToRelay,
originChainId: consts.originChainId.toString(),
destinationChainId: consts.destinationChainId.toString(),
realizedLpFeePct: consts.realizedLpFeePct,
relayerFeePct: consts.depositRelayerFeePct,
depositId: consts.firstDepositId.toString(),
message: erc20Message,
},
relayData: erc20LeafRelayData,
payoutAdjustmentPct: ethers.utils.parseEther("9").toString(), // 10x payout.
});
slowFills.push({
relayData: { ...erc20LeafRelayData, message: "0x" },
payoutAdjustmentPct: ethers.utils.parseEther("9").toString(), // 10x payout.
});

// WETH
const wethLeafRelayData = {
depositor: depositor.address,
recipient: recipient.address,
destinationToken: weth.address,
amount: consts.amountToRelay,
originChainId: consts.originChainId.toString(),
destinationChainId: consts.destinationChainId.toString(),
realizedLpFeePct: consts.realizedLpFeePct,
relayerFeePct: consts.depositRelayerFeePct,
depositId: consts.firstDepositId.toString(),
message: wethMessage,
};
slowFills.push({
relayData: {
depositor: depositor.address,
recipient: recipient.address,
destinationToken: weth.address,
amount: consts.amountToRelay,
originChainId: consts.originChainId.toString(),
destinationChainId: consts.destinationChainId.toString(),
realizedLpFeePct: consts.realizedLpFeePct,
relayerFeePct: consts.depositRelayerFeePct,
depositId: consts.firstDepositId.toString(),
message: wethMessage,
},
relayData: wethLeafRelayData,
payoutAdjustmentPct: ethers.utils.parseEther("-0.5").toString(), // 50% payout.
});
slowFills.push({
relayData: { ...wethLeafRelayData, message: "0x" },
payoutAdjustmentPct: ethers.utils.parseEther("-0.5").toString(), // 50% payout.
});

Expand Down Expand Up @@ -296,7 +306,7 @@ describe("SpokePool Slow Relay Logic", async function () {
consts.amountToRelay,
undefined,
undefined,
erc20Message
"0x"
).relayData,
partialAmountPostFees, // Set post fee amount as max amount to send so that relay filled amount is
// decremented by exactly the `partialAmount`.
Expand All @@ -317,9 +327,14 @@ describe("SpokePool Slow Relay Logic", async function () {
consts.depositRelayerFeePct,
consts.firstDepositId,
0,
erc20Message,
"0x",
ethers.utils.parseEther("9"),
tree.getHexProof(slowFills.find((slowFill) => slowFill.relayData.destinationToken === destErc20.address)!)
tree.getHexProof(
slowFills.find(
(slowFill) =>
slowFill.relayData.destinationToken === destErc20.address && slowFill.relayData.message === "0x"
)!
)
)
)
).to.changeTokenBalances(
Expand Down Expand Up @@ -353,7 +368,7 @@ describe("SpokePool Slow Relay Logic", async function () {
consts.amountToRelay,
undefined,
undefined,
wethMessage
"0x"
).relayData,
partialAmountPostFees,
consts.destinationChainId
Expand All @@ -374,9 +389,14 @@ describe("SpokePool Slow Relay Logic", async function () {
consts.depositRelayerFeePct,
consts.firstDepositId,
0,
wethMessage,
"0x",
ethers.utils.parseEther("-0.5"),
tree.getHexProof(slowFills.find((slowFill) => slowFill.relayData.destinationToken === weth.address)!)
tree.getHexProof(
slowFills.find(
(slowFill) =>
slowFill.relayData.destinationToken === weth.address && slowFill.relayData.message === "0x"
)!
)
)
)
).to.changeTokenBalances(weth, [spokePool], [slowFillAmountPostFees.div(2).mul(-1)]);
Expand Down Expand Up @@ -406,7 +426,7 @@ describe("SpokePool Slow Relay Logic", async function () {
consts.amountToRelay,
undefined,
undefined,
wethMessage
"0x"
).relayData,
partialAmountPostFees,
consts.destinationChainId
Expand All @@ -427,9 +447,14 @@ describe("SpokePool Slow Relay Logic", async function () {
consts.depositRelayerFeePct,
consts.firstDepositId,
0,
wethMessage,
"0x",
ethers.utils.parseEther("-0.5"),
tree.getHexProof(slowFills.find((slowFill) => slowFill.relayData.destinationToken === weth.address)!)
tree.getHexProof(
slowFills.find(
(slowFill) =>
slowFill.relayData.destinationToken === weth.address && slowFill.relayData.message === "0x"
)!
)
)
)
).to.changeEtherBalance(recipient, slowFillAmountPostFees.div(2));
Expand Down
2 changes: 2 additions & 0 deletions test/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ export const totalPostModifiedFeesPct = toBN(oneHundredPct).sub(toBN(modifiedRel

export const amountToRelayPreFees = toBN(amountToRelay).mul(toBN(oneHundredPct)).div(totalPostFeesPct);

export const amountToDepositPostFees = toBN(amountToDeposit).mul(toBN(totalPostFeesPct)).div(oneHundredPct);

export const amountToRelayPreModifiedFees = toBN(amountToRelay).mul(toBN(oneHundredPct)).div(totalPostModifiedFeesPct);

export const amountToRelayPreLPFee = amountToRelayPreFees.mul(oneHundredPct.sub(realizedLpFeePct)).div(oneHundredPct);
Expand Down