Restricted Router Withdrawals

[LayneHaber]

Summary

The current contracts allow a compromised router to drain their onchain liquidity to any arbitrary address. Instead, provide a feature where routers can optionally set their recipient address. If this is not set, routers are able to withdraw liquidity to anywhere.

Motivation

If a routers signing key is compromised, the attacker could drain the liquidity stored on the contract and send it to any specified address. This effectively means the key is in control of all unused liquidity on chain, which prevents router operators from adding large amounts of liquidity directly to the contract. Routers should be able to delegate the safe withdrawal address of any unused liquidity, creating a separation of concerns between router key and liquidity safety.

Proposed Solution

The contract should expose the following interface and mappings:

private mapping(address => address) routerRecipients;
private mapping(address => address) routerOwners;
private mapping(address => address) proposedRouterOwners;

// Sets the designated recipient for a router
public setRecipient(address _recipient, address _router) {
        // get existing recipient, router should only be able to set
        // this once otherwise if router key compromised, no problem
        // is solved since attacker could just update recipient
       address existing = routerOwners[_router];
      
       if (existing == address(0)) {
          require(msg.sender == _router, "Only router");
       } else {
          require(msg.sender == existing, "Only router owner");
       }

	// set recipient
	routerRecipients[_router] = _recipient
}

// Current owner or router may propose a new router owner
public proposeRouterOwner(address _proposed, address _router) {
        // get existing owner, only they should be able to set
       address existing = routerOwners[_router];
      
       if (existing == address(0)) {
          require(msg.sender == _router, "Only router");
       } else {
          require(msg.sender == existing, "Only owner");
       }

	// set proposed owner
	proposedRouterOwners[_router] = _proposed;
}

// New router owner must accept role, or previous if proposed is 0x0
public acceptRouterOwner(address _owner, address _router) {
        // get existing proposal
       address proposed = proposedRouterOwners[_router];
      
       if (proposed == address(0)) {
          // Handle case where router never proposed an owner either
          address current = routerOwners[_router];
          require(msg.sender == (current == address(0) ? _router : owner), "Only current owner");
       } else {
          require(msg.sender == existing, "Only proposed owner");
       }

	// set owner
	routerOwners[_router] = _owner;
}

public removeLiquidity(address _recipient, address _router, ...) {
	...
	// transfer to specicfied recipient IFF recipient not sest
	address recipient = routerRecipients[_router]
	LibAsset.transfer(recipient == address(0) ? _recipient : _router, ...)
}

public setupRouter(address _router, address _owner, address _recipient) onlyOwner {
      // whitelist
      // set initial properties
}

It is important to allow the role of an owner to be burned when a router is in operation and impose no restrictions on the recipient (who may not have arbitrary code execution abilities)

Test Cases

The following cases should be implemented:

it("removing liquidity should send to router if no recipient", () => { ... })
it("removing liquidity should send to recipient if set", () => { ... })
it("should be able to propose a new owner", () => { ... })
it("should be able to accept a new owner", () => { ... })
it("should be able to propose a renunciation of ownership", () => { ... })
it("should be able to renounce ownership", () => { ... })
it("should be able to set recipient", () => { ... })
it("should fail if owner doesn't set recipient", () => { ... })
it("should fail if proposed owner doesn't accept ownership", () => { ... })

Outstanding Questions

• Probably want to break out renouncing into a separate function(s) proposeRenunciation vs acceptRenunciation but the ideas are the same

Tasks / PRs

• Implement the above function
• Add the test cases

[wanglonghong]

I think its a little bit better to have both functions.
1. setPendingRecipient(pendingRecipient),
msg.sender should be router
2. acceptPendingRecipient
if (currentRecipient == address(0)) require(msg.sender == pendingRecipient)
if (currentRecipient ! = address(0)) require(msg.sender) == currentRecipient

The reason I thought both functions is to confirm the recipient once more by calling acceptPendingRecipient on the recipient side.
Like this, It's just scratch rn. So anyone can think of better architecture. but I think we should have a handshake process between router address and recipient, new recipient and old recipient.

[LayneHaber]

This is a good idea, I agree. A bit more onerous, but worth it to set up properly. Something similar was recommended by auditors for the ProposedOwnable contract

[ArjunBhuptani]

I'm a little concerned about doing this because there's a high likelihood that recipient could be a contract. I'm not quite sure how the handshake could work if that were the case.

[LayneHaber]

Would happen with the owner in this case (pending below comment)

[ArjunBhuptani]

The goal of restricting router recipient is to give router operators better fund security by separating the risk management of the router's signer key from the custody of the funds themselves.

This means we need to make sure that routers can:

• Set recipient for their liquidity as part of setup
• Set routerOwner which can update recipient
• Allow optionally setting routerOwner to 0x0 so that router operators can ensure that they are not having to worry about the routerOwner key now instead of just router signer.

[LayneHaber]

Can include a setupRouter function that does all of this on behalf of routers when they are whitelisted, or they can do it themselves. Flow:

1. Whitelist router
2. Set recipient defines recipient (if nonzero)
3. Set owner defines owner (if nonzero)

At any point if the owner was not set, the router should be able to update themselves

[LayneHaber]

Updated to reflect above discussions

[LayneHaber]

Tracking in #820

[LayneHaber]

Done! Archiving