Hook allowlist to fix M-03

[PaulRBerg]

Context

We've recently run a public audit context on CodeHawks, and it was very much a fruitful exercise. An important finding is M-03: The caller of withdraw and renounce can skip callbacks, by sending less gas.

@sablier-labs/solidity brainstormed how to address the issue internally for a few days.

Now, after some deliberation, I have come up with this new design for the hooks, which will address both the Gas Bomb and the Skipping Hooks problem.

Proposal

• Remove all hooks except the recipient hooks
• Run hooks only to known, previously allowlisted contracts
• The allowlisting can be performed by the contract admin only
• The allowlisting is a 'one-way street' - an allowlisted recipient contract cannot be removed from the allowlist
• The allowlisting is protected by a query to IERC165-supportsInterface method which must be implemented.

Rationale

Generally, the point is to meet users where they are. So far, only the recipient hooks have garnered interest, e.g. Fjord Foundry who is building a staking contract for Sablier NFTs. We should optimize our protocol in light of this.

The design in this proposal enables the implementation of contracts that hold stream NFTs on behalf of users, such as vaults or staking contracts.

The allowlisting is irreversible in order to provide stronger immutability and decentralization guarantees. Once a recipient contract is allowlisted, integrators should NOT have to trust us to keep their contract on the allowlist. Imagine how bad it would be if our keys are hacked - the attacker could effectively freeze all assets in the staking protocol.

Furthermore, the supportsInterface method is there to preclude the contract admin from accidentally allowlisting contracts that do not implement Sablier hooks.

API Changes

SablierV2Lockup

Functions

• Add isAllowedToHook
• Add allowToHook
• Change the hooks in _cancel and withdraw to query the allowlist before running them
• Remove the try/catch block to revert the entire tx when the hook reverts
• Remove the sender hooks in _cancel, renounce, and withdraw

Note

As suggested by @smol-ninja here, we might have to ask the hook to return the function selector in order to guarantee that the Sablier function has sufficient gas to finish the execution of the tx. I'm currently investigating this.

Events

• Add AllowToHook

New Behavior

ISablierLockupRecipient

Note: I removed "V2" from this interface due to the upcoming package tethering.

• Inherit IERC165 interface

Risks

It is impossible to prevent the following risks without compromising on decentralization. To mitigate them, we will be prudent about what contracts get allowlisted, and maintain an updated list of allowlisted entities in our docs.

• Revert risk - the recipient hooks SHOULD be designed with no reverts.
• If the allowlisted contract is upgradeable, the owner could upgrade the implementation to not comply with ISablierLockupRecipient anymore. Or they could intentionally DoS all senders.
• Gas bomb risk: a contract that consumes a lot of gas could require the sender to pay a huge amount of gas for canceling the stream.
• If the allowlisted contract contains a delegatecall, they can run arbitrary logic and e.g. DoS or gas bomb the sender.

RFC

This proposal enables a safe and improved hook architecture in the Lockup protocol, all the while not compromising on immutability and decentralization.

Kindly asking @sablier-labs/everybody and the Fjord Foundry team for comments on this.

See the accompanying PR for implementation details: #951

[0xdeth]

Hey everyone,

I'm deth from EgisSec, the auditing team that reported this issue in the Codehawks contest.

About the changes/fix:

Removing the try/catch will allow the recipient to force revert _cancel. The allowlist should mitigate this, considering that the admin allows only TRUSTED addresses and said addresses don't go rogue.

Another potential issue with removing the try/catch is that the callback might revert due to incorrect implementation in the recipient address. This will be the implementors fault, not yours of course. Adding disclaimers to allowlisted addresses (I assume other protocols) is a good idea.

As you've mentioned here:

The allowlisting is irreversible in order to provide stronger immutability and decentralization guarantees. Once a recipient contract is allowlisted, integrators should NOT have to trust us to keep their contract on the allowlist. Imagine how bad it would be if our keys are hacked - the attacker could effectively freeze all assets in the staking protocol.

To be extra safe you could use a multisig/DAO/gnosis safe. I understand this complicates things further, but it is an option if you are concerned about this specific point.

All in all, I think the changes are good. It puts the control of the callbacks in your hands and in the recipient hands, which you have to allow. Presumably these are other protocols or addresses that you will make sure are TRUSTED.

About some of the comments from the PR and in this discussion:

Since the contracts which the hook is called on must be allowed by Sablier, why not generalize it and allow both sender/recipient calls. Thus, instead of ISablierRecipient to have a ISablierLockupHookImplementer interface?
If we keep the current name, why not rename it to ISablierLockupRecipient? we might have a different recipient interface for future contracts

This is more of a design choice. I personally think since you are allowlisting the recipient you can allowlist the sender as well. Again, you are in control of the allowlist, so this should be fine.

Generally, the point is to meet users where they are

I understand where you are coming from, but having that extra functionality isn't a bad idea for the future where you will have more users with different needs/ideas.

[PaulRBerg]

Thank you so much for taking the time to review the proposal, @0xdeth! We're glad that you like the proposal.

Adding disclaimers to allowlisted addresses (I assume other protocols) is a good idea.

Yeah. We will keep an updated list of allowlisted recipients on our docs site and potentially in our UI, too. CC @sablier-labs/frontend.

To be extra safe you could use a multisig/DAO/gnosis safe

We're already using Safe multisigs for our protocol admin accounts:

https://docs.sablier.com/concepts/governance

This is more of a design choice
...
having that extra functionality isn't a bad idea for the future where you will have more users with different needs/ideas

Answered below why we decided to implement only recipient hooks.

[PaulRBerg]

Answering @andreivladbrg's questions from here:

1. We won't allow sender hooks for two reasons: (i) meet users there they are, and (ii) start small. The hook allowlist is a non-trivial change.
2. Good idea to rename it to ISablierLockupRecipient. It's unlikely that the same recipient contract would handle both Lockup and Flow streams. And, if it does, it can inherit both interfaces.
3. I'm against moving the hook logic in Adminable because it is used in the periphery by MerkleLockup and this contract is explicitly meant to be a replacement for Ownable. If we decide to re-implement the hook logic in Flow, we can re-introduce the Base abstract, which we deleted a while ago.
4. Changing the type of the parameter passed to allowToHook from ISablierRecipient to address is a good idea.
5. Why not return a bool was answered by @smol-ninja here.

[PaulRBerg]

Implemented points no.2 and no. 5 in #951

[andreivladbrg]

because it is used in the periphery by MerkleLockup

oh, yeah, forgot about it

[smol-ninja]

This is a fantastic proposal, and I support it as a solution for Hooks' callback issue.

Besides the risks you've mentioned, there's an additional risk of execution failure. If the hook call reverts, the entire execution will revert. To mitigate this, when deciding on adding a contract to the allowlist, it MUST be ensured that recipient contracts are designed to avoid revert on hook calls.

[PaulRBerg]

Thanks, Shub.

You're right. I added this risk to the list in the OP.

it MUST be ensured that recipient contracts are designed to avoid revert on hook calls.

I would say SHOULD instead of MUST. There may be legitimate use cases for reverting.

[smol-ninja]

Agree.

[ggrievous3]

This is an amazing and quick proposal @PaulRBerg, we at Fjord have gone through this and it looks good to us.

Thank you for a prompt resolution on this issue.

[rmsrob]

The proposed changes represent a significant improvement to the security and functionality of the Sablier protocol. The focus on immutability and decentralization is appreciated, and the adjustments align well with the needs of our Fjord staking contract. Thank you for your efforts.

[PaulRBerg]

Closing as everybody is aligned on the benefits of this feature, which got implemented in Lockup v1.2.0

Thanks all for your contributions!