Implement proxy plugin

[andreivladbrg]

Depends on #28.

Addresses #19.

[PaulRBerg]

Great work. Specific feedback left in the comments below.

It may be clearer to rename the sender user to something else, e.g. Alice. It can be confusing to name it like that given that the proxy contract is the sender.

[andreivladbrg]

It may be clearer to rename the sender user to something else, e.g. Alice

For now, let's keep it as it is and sleep on this idea.

[PaulRBerg]

Fine, I will create a separate PR for changing the default proxy owner.

[PaulRBerg]

During my latest code review, I discovered an important issue with the current onStreamCanceled implementation.

Anyone can run a proxy plugin, not just the proxy owner. It would thus be a good idea to check that msg.sender is the Sablier contract to improve the plugin's security.

[PaulRBerg]

To follow up on my previous point - there are more issues with the current implementation of the proxy plugin.

• Anyone can call the onStreamCanceled function, and thus transfer assets from the proxy to the proxy owner by manipulating the input values. To prevent this, we must check that the stream's sender is address(this).
• The plugin does not inherit from PRBProxyStorage, but it should because the onStreamCanceled function is supposed to be called within the context of the proxy
• The onStreamCanceled function obtains the owner by calling the owner getter on the proxy - but the context in which this function is the proxy itself! Once we address the previous point, i.e., inherit from PRBProxyStorage, we will be able to access the owner as a storage variable in the proxy plugin

[PaulRBerg]

@andreivladbrg - I have finished re-implementing the proxy plugin in light of my feedback above. Can you please review it?

[andreivladbrg]

Thanks for the feedback.

Anyone can call the onStreamCanceled function, and thus transfer assets from the proxy to the proxy owner by manipulating the input values. To prevent this, we must check that the stream's sender is address(this).

I think you are talking about something like this:

  ISablierV2LockupSender(address(proxy)).onStreamCanceled(linear, streamId, users.recipient.addr, 10e18, 0);

and not to:

    plugin.onStreamCanceled(linear, streamId, users.recipient.addr, 10e18, 0);

The second version won't work and would do nothing. The first version can only work if the proxy contract holds the assets associated with the streamId.

The solution you suggested, i.e. sender != address(this), doesn't address the issue in the first version mentioned above. You can test it using this:

    function test_OnStreamCanceled() external {
        uint256 streamId = createWithRange();

        vm.warp(defaults.CLIFF_TIME());

        uint256 initialBalance = dai.balanceOf(users.sender.addr);

        expectCallToTransfer({ to: users.sender.addr, amount: 10e18 });

        deal({ token: address(dai), to: address(proxy), give: 10e18 });
        ISablierV2LockupSender(address(proxy)).onStreamCanceled(linear, streamId, users.recipient.addr, 10e18, 0);

        uint256 actualBalance = dai.balanceOf(users.sender.addr);
        uint256 expectedBalance = initialBalance + 10e18;
        assertEq(actualBalance, expectedBalance, "balances do not match");
    }

To really fix this we should make this check instead:

        if (msg.sender != address(lockup)) {
            revert Errors.InvalidCaller();
        }

Regarding inheriting PRBProxyStorage, both version would work, and neither would affect the security. To make a good decision, we should weigh the pros and cons of each approach.

A downside to inheriting PRBProxyStorage is the increased deployment cost. On the other hand, opting not to inherit it, maybe(not sure yet) it could result in higher gas for canceling a stream for the recipient.

I just realised a potentially more dangerous issue: Certain ERC20 implementations, such as OpenZeppelin, incorporate hooks for before- and after-transfer. I kept thinking about a malicious scenario involving re-entrancy in the fallback function, which could potentially result in losing funds. Although no specific instances come to mind at the moment but I believe it's worth investigating deeper into this.

[PaulRBerg]

I was actually referring to calling the plugin contract directly. Nonetheless, you made an excellent observation, i.e., that we should write another test for the case when a third party calls the onStreamCanceled hook directly via the proxy.

The second version won't work and would do nothing

Yes, because Sablier disregards reverts triggered by hooks, but we have to write tests to validate this behavior. And this is precisely what I did: see test_RevertWhen_PluginContractStreamSender.

if (msg.sender != address(lockup)) {
    revert Errors.InvalidCaller();
}

This doesn't work, unfortunately, because lockup is a user-provided parameter. It is possible to create a dummy contract that implements all SablierV2Lockup functions called in onStreamCanceled, and pass that as the value of lockup.

I suggest we merge this PR as is, and consider how to address this problem later - the only solution I can think of now is to add an allowlist of Sablier contract addresses, which would require us to inherit from Adminable in SablierV2ProxyPlugin.

Regarding inheriting PRBProxyStorage, both versions would work, and neither would affect the security

Correction: the material security of the proxy wouldn't be affected at the moment. However, from a best practices point of view, it is best to inherit from PRBProxyStorage because the plugin is always meant to be executed in the proxy's context. The storage layout of the plugin should always match that of the proxy contract. The gas cost is a moot point because the plugin is expected to be deployed rarely, and we are the ones who will deploy it.

To make a good decision, we should weigh the pros and cons of each approach.

It's pretty clear to me that inheriting from PBRProxyStorage is what we should do here, as per my explanation above.

it could result in higher gas for canceling a stream for the recipient

The final nail in the coffin 🙈

Certain ERC20 implementations, such as OpenZeppelin, incorporate hooks for before- and after-transfer

Great point. We should also be mindful of ERC-777 tokens.

I kept thinking about a malicious scenario involving re-entrancy in the fallback function
...
I believe it's worth investigating deeper into this.

I agree that it is good to be wary about this, but I also can't think of any security threat.

[PaulRBerg]

As we talked about on the phone, I need to make some clarifications because I made a few errors - thanks for correcting them, @andreivladbrg.

The first error is the name of the test_RevertWhen_PluginContractStreamSender test, which should be rewritten to drop the RevertWhen part because there is no revert in the test itself.

The second is my (very) ambiguous formulation here:

Anyone can call the onStreamCanceled function, and thus transfer assets from the proxy to the proxy owner by manipulating the input values
...
I was actually referring to calling the plugin contract directly.
...
Yes, because Sablier disregards reverts triggered by hooks

I made a mush out of the plugin's behavior.

There are three separate points:

1. Calling the plugin directly via Sablier - the tests already handle this scenario, which we need to test because the plugin is a sensible contract that must not be tampered with (otherwise, a significant number of users would have to reinstall the plugin).
2. Calling the plugin directly without going through either the proxy or Sablier - the tests do not handle this scenario yet, but they should due to the same explanation as in the first point.
3. Calling the plugin directly via the proxy, without Sablier - this scenario is not handled by the tests, but it should be, as you correctly pointed out.

[PaulRBerg]

FYI, @andreivladbrg, I have accidentally merged #35 into this PR's head branch.

This is not ready to review just yet, though. I will let you know.

[PaulRBerg]

This PR has become cluttered with discussions and misconceptions (many of which have been on my part), so I created another PR (#38) to start afresh. There have also been quite a few structural changes that have deviated from the original design laid out in #19.