fix(SpokePool): Enforce exclusivity consistently #649
Conversation
The new relative exclusivity check has not been propagated to fillV3RelayWithUpdatedDeposit(). Identified via test case failures in the relayer. Signed-off-by: Paul <[email protected]>
pxrl
changed the title
contracts/SpokePool.sol
Outdated
| if ( | ||
| relayData.exclusiveRelayer != msg.sender && | ||
| relayData.exclusivityDeadline >= getCurrentTime() && | ||
| relayData.exclusiveRelayer != address(0) | ||
| ) { | ||
| revert NotExclusiveRelayer(); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I factored out the common logic to a _fillIsExclusive() function. I think this is probably the right call but could be persuaded otherwise.
There was a problem hiding this comment.
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
| @@ -222,7 +222,6 @@ interface V3SpokePoolInterface { | |||
| error DisabledRoute(); | |||
| error InvalidQuoteTimestamp(); | |||
| error InvalidFillDeadline(); | |||
| error InvalidExclusiveRelayer(); | |||
There was a problem hiding this comment.
Driveby change - this error is unused. It was dropped with the recent changes to permit relative exclusivity.
Added tests here: 6b5f5f9 |
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.