Skip to content
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

feat: Enable pausing deposits and fills unilaterally #212

Merged
merged 4 commits into from
Dec 30, 2022
Merged

feat: Enable pausing deposits and fills unilaterally #212

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec030d0
feat: Enable pausing deposits and fills unilaterally
nicholaspai Dec 20, 2022
56f93e7
Merge branch 'master' of https://github.com/across-protocol/contracts…
nicholaspai Dec 27, 2022
6a13915
Fix tests
nicholaspai Dec 27, 2022
290d3ec
Update SpokePool.sol
nicholaspai Dec 30, 2022
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
39 changes: 35 additions & 4 deletions contracts/SpokePool.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
// Count of deposits is used to construct a unique deposit identifier for this spoke pool.
uint32 public numberOfDeposits;

// Whether deposits and fills are disabled.
bool public pausedFills;
bool public pausedDeposits;

// This contract can store as many root bundles as the HubPool chooses to publish here.
RootBundle[] public rootBundles;

Expand Down Expand Up @@ -121,6 +125,8 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
address caller
);
event EmergencyDeleteRootBundle(uint256 indexed rootBundleId);
event PausedDeposits(bool indexed isPaused);
event PausedFills(bool indexed isPaused);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Do these need to be indexed? I imagine that there will be so few of them that there's not much advantage to filtering on the "paused" state.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1


/**
* @notice Construct the base SpokePool.
Expand Down Expand Up @@ -151,10 +157,35 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
_;
}

modifier unpausedDeposits() {
require(!pausedDeposits, "Paused deposits");
_;
}

modifier unpausedFills() {
require(!pausedFills, "Paused fills");
_;
}

/**************************************
* ADMIN FUNCTIONS *
**************************************/

/**
* @notice Pauses deposit and fill functions. This is intended to be used during upgrades or when
* something goes awry.
* @param pause true if the call is meant to pause the system, false if the call is meant to unpause it.
*/
function pauseDeposits(bool pause) public override onlyAdmin nonReentrant {
pausedDeposits = pause;
emit PausedDeposits(pause);
}

function pauseFills(bool pause) public override onlyAdmin nonReentrant {
pausedFills = pause;
emit PausedFills(pause);
}

/**
* @notice Change cross domain admin address. Callable by admin only.
* @param newCrossDomainAdmin New cross domain admin.
Expand Down Expand Up @@ -250,7 +281,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
uint256 destinationChainId,
uint64 relayerFeePct,
uint32 quoteTimestamp
) public payable override nonReentrant {
) public payable override nonReentrant unpausedDeposits {
// Check that deposit route is enabled.
require(enabledDepositRoutes[originToken][destinationChainId], "Disabled route");

Expand Down Expand Up @@ -317,7 +348,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
uint64 newRelayerFeePct,
uint32 depositId,
bytes memory depositorSignature
) public override nonReentrant {
) public override nonReentrant unpausedDeposits {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it desirable to make speedups subject to deposits not being paused?

As I see it, the point of pausing deposits is to stop additional funds from entering the Spoke Pool. In the case of a speedup, the funds are already there, and are presumably stuck because the deposit was never filled on the destination chain.

Additionally, as far as I can see, the only side-effect of a speedup is that the Spoke Pool emits another event. It's up to the bots as to whether they accept a speedup event, and it will implicitly be conditional on fills being enabled on the destination chain anyway.

Based on the above, I feel like we should not apply this modifier to speedUpDeposit(). wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm I think its a bit easier to reason about from the bot's perspective of all deposit related events stop once a contract is paused, even though its philosophically ok to not pause speed ups. I was thinking pausing deposits and fills is more of a bot convenience so the owner of the contracts can effectively downgrade a contract by stopping all of its events

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough - I can definitely see the argument for keeping it simple.

There is a theoretical possibility for the edge case I described, and an annoying scenario would be that we have a number of underpriced deposits that are suddenly marooned in the Spoke Pool. In this case we'd need to either unpause to allow speedups to be submitted, or just take the hit and fill these relays at a discount. In theory the relayerFeePct could be very very low.

It's a theoretical possibility, and probably unlikely to be a reality. As long as we're OK with the possibility of it arising then I have no additional input on this one :)

Copy link
Member Author

@nicholaspai nicholaspai Dec 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

hmmm ok that's a valid concern, i don't love the idea that someone will try to use the speedup signatures to replay attack but I guess its not really an attack since they could have chosen to call speedUp on either the old or the new spoke pool at the same cost with no extra benefit

require(newRelayerFeePct < 0.5e18, "invalid relayer fee");

_verifyUpdateRelayerFeeMessage(depositor, chainId(), newRelayerFeePct, depositId, depositorSignature);
Expand Down Expand Up @@ -367,7 +398,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
uint64 realizedLpFeePct,
uint64 relayerFeePct,
uint32 depositId
) public nonReentrant {
) public nonReentrant unpausedFills {
// Each relay attempt is mapped to the hash of data uniquely identifying it, which includes the deposit data
// such as the origin chain ID and the deposit ID, and the data in a relay attempt such as who the recipient
// is, which chain and currency the recipient wants to receive funds on, and the relay fees.
Expand Down Expand Up @@ -423,7 +454,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
uint64 newRelayerFeePct,
uint32 depositId,
bytes memory depositorSignature
) public override nonReentrant {
) public override nonReentrant unpausedFills {
_verifyUpdateRelayerFeeMessage(depositor, originChainId, newRelayerFeePct, depositId, depositorSignature);

// Now follow the default fillRelay flow with the updated fee and the original relay hash.
Expand Down
4 changes: 4 additions & 0 deletions contracts/SpokePoolInterface.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ interface SpokePoolInterface {
bool enable
) external;

function pauseDeposits(bool pause) external;

function pauseFills(bool pause) external;

function setDepositQuoteTimeBuffer(uint32 buffer) external;

function relayRootBundle(bytes32 relayerRefundRoot, bytes32 slowRelayRoot) external;
Expand Down
16 changes: 16 additions & 0 deletions test/SpokePool.Admin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,22 @@ describe("SpokePool Admin Functions", async function () {
expect(await spokePool.depositQuoteTimeBuffer()).to.equal(60);
});

it("Pause deposits", async function () {
expect(await spokePool.pausedDeposits()).to.equal(false);
await expect(spokePool.connect(owner).pauseDeposits(true)).to.emit(spokePool, "PausedDeposits").withArgs(true);
expect(await spokePool.pausedDeposits()).to.equal(true);
await expect(spokePool.connect(owner).pauseDeposits(false)).to.emit(spokePool, "PausedDeposits").withArgs(false);
expect(await spokePool.pausedDeposits()).to.equal(false);
});

it("Pause fills", async function () {
expect(await spokePool.pausedFills()).to.equal(false);
await expect(spokePool.connect(owner).pauseFills(true)).to.emit(spokePool, "PausedFills").withArgs(true);
expect(await spokePool.pausedFills()).to.equal(true);
await expect(spokePool.connect(owner).pauseFills(false)).to.emit(spokePool, "PausedFills").withArgs(false);
expect(await spokePool.pausedFills()).to.equal(false);
});

it("Delete rootBundle", async function () {
await spokePool.connect(owner).relayRootBundle(mockRelayerRefundRoot, mockSlowRelayRoot);

Expand Down
19 changes: 19 additions & 0 deletions test/SpokePool.Deposit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,25 @@ describe("SpokePool Depositor Logic", async function () {
});
it("Depositing ERC20 tokens correctly pulls tokens and changes contract state", async function () {
const currentSpokePoolTime = await spokePool.getCurrentTime();

// Can't deposit when paused:
await spokePool.connect(depositor).pauseDeposits(true);
await expect(
spokePool
.connect(depositor)
.deposit(
...getDepositParams(
recipient.address,
erc20.address,
amountToDeposit,
destinationChainId,
depositRelayerFeePct,
currentSpokePoolTime
)
)
).to.be.reverted;
await spokePool.connect(depositor).pauseDeposits(false);

await expect(
spokePool
.connect(depositor)
Expand Down
6 changes: 6 additions & 0 deletions test/SpokePool.Relay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ describe("SpokePool Relayer Logic", async function () {
destErc20.address
);

// Can't fill when paused:
await spokePool.connect(depositor).pauseFills(true);
await expect(spokePool.connect(relayer).fillRelay(...getFillRelayParams(relayData, consts.amountToRelay))).to.be
.reverted;
await spokePool.connect(depositor).pauseFills(false);

await expect(spokePool.connect(relayer).fillRelay(...getFillRelayParams(relayData, consts.amountToRelay)))
.to.emit(spokePool, "FilledRelay")
.withArgs(
Expand Down