Skip to content

Conversation

@pxrl
Copy link
Contributor

@pxrl pxrl commented Oct 9, 2024

The new relative exclusivity check has not been propagated to fillV3RelayWithUpdatedDeposit() or requestV3SlowFill(). Identified via test case failures in the relayer.

This is a very-low criticality issue. It's very unlikely that any relayer would be trying to call fillV3RelayWithUpdatedDeposit() within the exclusivity window. There's a slightly higher chance of hitting this issue on slow fill requests because the RL relayer is fast enough to hit the probable exclusivity windows on some routes for larger transfers, which is where a token shortfall is more likely.

The new relative exclusivity check has not been propagated to
fillV3RelayWithUpdatedDeposit(). Identified via test case failures in
the relayer.

Signed-off-by: Paul <108695806+pxrl@users.noreply.github.com>
@pxrl pxrl requested review from bmzig, mrice32 and nicholaspai October 9, 2024 22:00
@pxrl pxrl changed the title fix(SpokePool): Apply exclusivity consistently fix(SpokePool): Enforce exclusivity consistently Oct 9, 2024
Comment on lines 888 to 894
if (
relayData.exclusiveRelayer != msg.sender &&
relayData.exclusivityDeadline >= getCurrentTime() &&
relayData.exclusiveRelayer != address(0)
) {
revert NotExclusiveRelayer();
}
Copy link
Contributor Author

@pxrl pxrl Oct 9, 2024

Choose a reason for hiding this comment

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

We now have this check in 2 places. I'm tempted to factor it out to a separate function; it'd be nice to also bundle the requestedV3SlowFill() check as well but it's slightly less strict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I factored out the common logic to a _fillIsExclusive() function. I think this is probably the right call but could be persuaded otherwise.

Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

Dang good catch. I think refactoring to shared internal function could make sense

Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

Can you add unit tests to these functions similar to these so we can prevent regressions? https://github.com/across-protocol/contracts/blob/master/test/evm/hardhat/SpokePool.Relay.ts#L330

error DisabledRoute();
error InvalidQuoteTimestamp();
error InvalidFillDeadline();
error InvalidExclusiveRelayer();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Driveby change - this error is unused. It was dropped with the recent changes to permit relative exclusivity.

@pxrl
Copy link
Contributor Author

pxrl commented Oct 11, 2024

Can you add unit tests to these functions similar to these so we can prevent regressions? https://github.com/across-protocol/contracts/blob/master/test/evm/hardhat/SpokePool.Relay.ts#L330

Added tests here: 6b5f5f9

@nicholaspai nicholaspai added the do not merge do not merge label Oct 22, 2024
@nicholaspai nicholaspai merged commit 05302cb into master Oct 30, 2024
9 checks passed
@nicholaspai nicholaspai deleted the pxrl/exclusivity branch October 30, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge do not merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants