Codebase feedback and questions (Gabriel, March '23)

[razgraf]

Some questions and feedback for the current state of the contracts as of #2de2f40.

SablierV2Adminable

→ transferAdmin

Similar to an optimization made in prb-proxy I suggest getting rid of the oldAdmin placeholder. The transfer method can only be accessed by msg.sender which is in this situation the old admin. No need to use that extra contextual variable.

https://github.com/sablierhq/v2-core/blob/d7f46c1212ebf9e56b93dfa1acec910b0193e0e9/src/abstracts/SablierV2Adminable.sol#L34-L42

SablierV2Lockup

→ nftDescriptor

The logic of having a separate contract for the descriptor was (aside from separation of concerns and freezing purposes) to be able to fix visual bugs or update it later. I see that we're storing the descriptor address in an immutable value with no possibility of updating it later on. I suggest we implement a setter for this value.

https://github.com/sablierhq/v2-core/blob/d7f46c1212ebf9e56b93dfa1acec910b0193e0e9/src/abstracts/SablierV2Lockup.sol#L27

→ withdrawMultiple

The permission schema for "who can withdraw" (from an item) is (a) a recipient, (b) an authorized person or (c) the sender, as long as the withdraw is directed to the current recipient. (C) is very smart as it allows the sender to pay for the transaction, thus relieving the recipient from that cost.

The "multiple" version unfortunately limits this ability to just the recipient. Some notes:
I think the name of the function is a bit misleading in the sense that withdrawMultiple != x * withdraw .
We could use structs for this one, if gas costs allow it (as in a struct containing {streamId, to, amount} ) mainly for consistency - a nice side effect: it would unlock the ability to target separate account (separate to)

→ maxFee

I see that we're using the same MAX_FEE value for both broker fees and protocol fees. This again is a good place to either split the two or make sure we explain in the docs that the actual maximum fee you could pay from your principal is 2 * MAX_FEE (broker + protocol).

https://github.com/sablierhq/v2-core/blob/d7f46c1212ebf9e56b93dfa1acec910b0193e0e9/src/libraries/Helpers.sol#L29-L40

SablierV2Config, SablierV2LockupLinear, SablierV2Comptroller and SablierV2NftDescriptor

These look good and I haven't found any issues with them (neither logical nor subjective). Complex actions will most of the times benefit from a check against the initial deposit amount, so "extracting" money from outside of a stream seems impossible at the moment, which is a good thing.

SablierV2LockupPro, FlashLoans

Review on-going.

[andreivladbrg]

→ SablierV2Adminable
That’s correct. I remember mentioning this optimization in a PR, but I can't find it to see what @PaulRBerg's reasoning was for keeping it this way. I support this optimization.

→ nftDescriptor
Paul and I discussed this, and we decided to use an immutable variable to reduce the number of lines of code, which would make the audit less expensive.

→ withdrawMultiple
We had many discussions and code changes regarding this topic. A few months ago, we had multiple versions of the withdraw functions: withdraw, withdrawTo, withdrawMultiple, and withdrawToMultiple. The "multiple" versions respected the "x * withdraw." The difference between them was that the "to" versions could only be called by the recipient or an approved operator, while the simple ones could also be called by the sender. The main issue with having so many withdraw functions was the amount of repetitive code, and as we began implementing the fee system and flash loans, we faced issues with contract size (which is limited to 24.576kb on the Ethereum blockchain), even if the contracts were compiled with "via_ir". We had to merge them. As for why the multiple version doesn't allow the sender to call it, you can check the comments in this PR for more details (TLDR: the recipient can {ERC721-setApprovalForAll} on the sender). Using a struct for parameters is an interesting idea, have to sleep on it.

Note: I was in favor of enabling the sender to call the "multiple", which would have made the name not misleading.

→ maxFee

explain in the docs that the actual maximum fee you could pay from your principal is 2 * MAX_FEE (broker + protocol).

Good point, we should explain this in our docs.

[PaulRBerg]

Great feedback, thanks, @razgraf!

→ transferAdmin

I suggest getting rid of the oldAdmin placeholder .. No need to use that extra contextual variable.

Good catch. We will remove that placeholder variable, and save a little gas by doing so.

→ nftDescriptor

The logic of having a separate contract for the descriptor was ... to be able to fix visual bugs or update it later. I suggest we implement a setter for this value

I agree. As you said on Slack, having the ability to swap out the NFT descriptor contract would be helpful if and when (i) we want to launch without the NFT art and (ii) there is a bug in the SVG generator algorithm.

We will implement an admin-callable _setNFTDescriptor function.

→ withdrawMultiple

(C) is very smart as it allows the sender to pay for the transaction, thus relieving the recipient from that cost.

(C) doesn't have just an economic benefit. The more important aspect is that it is a permissionless escape-hatch in case the recipient is an entity that can neither interact with Sablier contracts nor approve an ERC-721 operator (e.g., a Binance address).

I think the name of the function is a bit misleading in the sense that withdrawMultiple != x * withdraw

I don't follow this, i.e., I don't see why the name is misleading. Could you explain what x is in your equation? withdrawMultiple actually does what your equation says, i.e. it iterates over an array of stream ids and calls _withdraw on each stream.

The noun that "multiple" is attached to is "streams", and this is indicated by the name of the first parameter:streamIds.

We've iterated on the name of this function quite a bit; it used to be called withdrawAll, which was worse because it could have been confused with withdrawMax.

The "multiple" version unfortunately limits this ability to just the recipient

That is fine. As explained above, the main advantage of allowing the sender to withdraw is protection against cases where stream recipients are addresses that cannot interact with Sablier.

It's unlikely that all recipients of a batch stream creation are invalid, but even if the unlikely case that this happens, the sender can call withdraw on each stream and call it a day. We should not engineer the feature set of the contracts in light of the most unlikely things that can happen.

We could use structs for this one, if gas costs allow it (as in a struct containing {streamId, to, amount} ) mainly for consistency - a nice side effect: it would unlock the ability to target separate account (separate to)

Gas cost is not the primary concern here. Security, maintenance, and fitness-for-purpose are. @andreivladbrg addressed the first two points with his answer; I will tackle the last point.

How useful would it be to have this functionality to withdraw to multiple accounts from multiple streams? What monster of a UI form would that be? I would rather invest that frontend effort into building airstreams and V3. Users can withdraw from V2 to one account, and then parcel out their tokens as they wish with MetaMask. There's no need to reinvent the wheel in our own contracts.

→ maxFee

This again is a good place to either split the two or make sure we explain in the docs that the actual maximum fee you could pay from your principal is 2 * MAX_FEE (broker + protocol).

As hinted at here, there are gas optimization and security reasons for reusing the MAX_FEE constant for both the protocol and the broker fees. Thus, we will definitely not split them.

But concerning the docs, I agree. This is the perfect use case for a diagram where the protocol and the broker fee are visually stacked.

→ others

These look good and I haven't found any issues with them

Noice.

so "extracting" money from outside of a stream seems impossible at the moment

One would hope so! We have an invariant test that gives us confidence that this is the case:

https://github.com/sablierhq/v2-core/blob/d7f46c1212ebf9e56b93dfa1acec910b0193e0e9/test/invariant/lockup/Lockup.t.sol#L42-L61

Review on-going.

I suggest you skim through EIP-3156 since all the SablierV2FlashLoan contract does is follow the ERC spec.

In the pro contract, you may be particularly interested to read the NatSpec of the streamedAmountOf function, as well as the implementations of the _calculateStreamedAmountForMultipleSegments and the _calculateStreamedAmountForOneSegment functions.

Finally, to get a better handle on the pro contract, I also recommend you install the Copilot Labs extension and use its "Explain" feature. It's the perfect tool for this job.

[PaulRBerg]

Update about the access control of withdrawMultiple - due to reasons unrelated to the discussion herein, in PR #467 I refactored the withdrawMultiple function by allowing the stream's sender to call it.

[PaulRBerg]

Closing, as we've settled and/ or responded to all points.