feat: staking template for Sablier NFTs

[smol-ninja]

Changelog

• Closes Add a staking template for Sablier NFTs #8

Note

This template is supposed to help integrators build staking contracts using Sablier NFT and not use it as it is. I have written the contract with certain assumptions and the actual implementation depends on the specific requirements of the integrators.

[PaulRBerg]

I'll let @andreivladbrg review this one.

Shouldn't we get it audited, though, before merging it on main?

Maybe we should put it on staging.

[smol-ninja]

Shouldn't we get it audited, though, before merging it on main?

• If an integrator is using this template in their core protocol, isn't it his responsibility to get the code audited?
• I do not expect others to use it directly since the staking requirements could be different for them. That's why this is a template.

If we want others to use it as it is directly, then let me know, I will add detailed tests to cover all branches (currently they only test basic functionalities).

[PaulRBerg]

A bug in this template would reflect pretty badly on Sablier. The tests would at least provide us with some comfort.

In any case, we need to add a big warning/ caveat in the template file itself that Sablier does not give any warranties, linking to our LPGL license.

[smol-ninja]

I have changed my mind about the security of the contract and I now agree with you @PaulRBerg. So, I have added proper tests for the staking contract. We can get a brief review of this contract to identify any exploitable bug but otherwise I think a full audit would not be necessary.

But in any case, this template is supposed to help integrators build staking contracts using Sablier NFT and not use it as it is. I have written the contract with certain assumptions and the actual implementation depends on the specific requirements of the integrators.

I have added the following disclaimer as well:

/// @notice DISCLAIMER: This template has not been audited and is provided "as is" with no warranties of any kind,
/// either express or implied. It is intended solely for demonstration purposes on how to build a staking contract
/// using Sablier NFT. This template should not be used in a production environment. It makes specific assumptions that
/// may not be applicable to your particular needs.

@andreivladbrg can you please review whenever you have time?

[PaulRBerg]

Thanks, @smol-ninja. Great work.

Agree that a light review would be nice - maybe Rusty can do us a solid and take a look?

A couple of todos after this is merged and reviewed:

• Mention this on the docs website somewhere (maybe under "Contracts → V2 Protocol → Staking"?)
• Work with Max to notify all users who have inquired about staking that we now offer a template for this and that we're looking for feedback

[andreivladbrg]

Thank you Shub for the PR, and I am sorry for the delayed review. I took my time to understand the staking protocols
that are out there and how they work, mostly Synthetix, which is the biggest I found (I am not considering Lido as it is not similar to what we want to achieve - staking ERC20 tokens).

---

Notes: You may ignore the comments I left on the code. I stopped adding them once I better understood the code.
Let's wait until we reach an agreement on the necessary changes.

I have not yet looked at the tests.

I started working locally on a new version but paused until we reach an agreement.

---

From my understanding, and after looking at synthetix contracts, staking is made at a fixed range (start/end period). The reward rate is calculated based on the total amount available to be distributed and the duration.

Having said that, here are some problems with this template:

• an expiration for when the rewards end is missing
• a total amount staked is missing
• the updateClaimAmount function does not consider that there could be multiple stakers
  • the reward rate is global, it should be divided by the total amount staked if there are multiple stakers
• updateClaimAmount function behaves as if the staker has access to the full amount streamed at each moment in time
  • the staking period may end before the stream ends, meaning the staker will generate rewards with tokens that are not theirs by the end of the staking campaign

---

So I propose the design and following changes:

• allow only non-canceleable streams to be staked
• allow only streams with endTime < stakingEndTime
• make the additional calculations in the stake function to calculate the amount "deposited" by staker (balance)
• add a totalAmountStaked variable that is going to be used in calculation function
• implement the onStreamWithdrawn hook that updates a state variable
• address the issue in updateClaimAmount and take into consideration the withdrawn tokens that are updated in storage
• ensure that claiming the withdrawn assets updates the totalAmountStaked and staker's balance
• add an archive of Sablier contracts that are allowed to be used

---

Also, here are some videos that initially helped me understand staking:

• https://youtu.be/rXuDelwHLoo?si=j-yBbf1XhQMvUnOo
• https://youtu.be/32n3Vu0BK4g?si=W0lryunaiAMZUIJF

[smol-ninja]

Thanks for the detailed feedback @andreivladbrg. I appreciate you taking the time through it. Please find my comments below:

I agree with the following:

• There should be an end time for staking rewards. I initially didn't add it because I thought of it as a user-specific requirement rather than a common requirement. But after reading your review, I have changed my mind about it.
• There should be a tracker for the total amount staked.
• I implemented an example of a fixed reward. However, I agree that this is not a realistic example. I will update it such that APY is calculated based on the deposited rewards and the total amount staked.

allow only streams with endTime < stakingEndTime

I don't understand why we need this inequality. Do you mean to say that if stream endTime is beyond stakingEndTime, it should not be allowed? Why?

Alternatively, after staking ends, streams should not be allowed to stake anymore and rewards will stop accumulating. Is that what you meant?

Allow only non-cancelable streams to be staked

I do not agree with this. The objective of this template is to provide knowledge on how Sablier NFT can be staked. What if a user wants to allow staking for cancelable streams? That is the reason I added the isCanceled branch in the updateClaimAmount function. Ofc we don't encourage this but I see it as a user-specific requirement.

Implement the onStreamWithdrawn hook that updates a state variable

That is a good approach. I failed to realize that I could also make use of hooks.

Address the issue in updateClaimAmount and take into consideration the withdrawn tokens that are updated in the storage
ensure that claiming the withdrawn assets updates the total amount staked and staked's balance

Agree

Add an archive of Sablier contracts that are allowed to be used

Agree. Since the rewards are stored against tokenId which could be same for both LockupLinear and LockupDynamic, whitelisting would be necessary. And, I should also add it to the assumption that both Linear and Dynamic can not be allowed at the same time because they can have the same tokenId.

Please let me know if I have not addressed any point that you have raised.

---

One more thing I want to mention is that "how rewards are updated" is a user-specific requirement. For example, Aevo updates rewards through the merkle tree. So they always have a surplus but the APY is calculated based on the Merkle tree and not based on the total rewards / total staked ratio.

Similarly, different protocols can have their requirement for distribution which is entirely up to them. I don't know what is a more popular approach. Synthetix is OG but I don't think everyone follows their contract. For example, I was looking at the Aave staking contract and it is differently designed than Synthetix.

But for this template, we can use Synthetix approach.

[andreivladbrg]

I don't understand why we need this inequality. Do you mean to say that if stream endTime is beyond stakingEndTime, it should not be allowed? Why?

My rationale for not allowing staking with streams that have an endTime beyond stakingEndTime is that the individual doing this would essentially be rewarding the stakers with funds that belong to them by the end of the staking campaign.
For example, assuming two people have an ongoing stream of the same amount, where one stream has an endTime < stakingEndTime and the other has an endTime > stakingEndTime, it wouldn’t be fair for the person whose stream ends before the staking campaign finishes to generate the same rewards as the other.

Another option that came to mind is to implement a custom withdrawableAmountOf function that takes a time variable as a parameter, which would be the stakingEndTime, to calculate how much one would have by the end of the campaign.

Alternatively, after staking ends, streams should not be allowed to stake anymore and rewards will stop accumulating. Is that what you meant?

Well after staking ends if there are no longer tokens in the contracts, yes, no one will generate rewards anymore. (no matter what kind of stream you are staking)

I do not agree with this. The objective of this template is to provide knowledge on how Sablier NFT can be staked. What if a user wants to allow staking for cancelable streams?

I understand, but I am in favor of informing the user that in the current template we only allow non-cancelable streams for security reasons, and of providing them with a potential direction to integrate cancelable streams. This approach would enable users to come up with something creative.

That is the reason I added the isCanceled branch in the updateClaimAmount function.

I see, but I don't think this would work with the idea of having a "deposit" amount that is calculated at the moment of staking. What would that amount be and how is going to be calculated?

That is a good approach

thanks, glad to hear

tokenId which could be same for both LockupLinear and LockupDynamic, whitelisting would be necessary. And, I should also add it to the assumption that both Linear and Dynamic can not be allowed at the same time because they can have the same tokenId

That's an issue that I had not thought about; thank you for mentioning it.

We could modify the mapping as follows:

mapping(address nftContract=> mapping(uint256 tokenId => uint256 amount)) public stakingRewards;

wdyt? would this fix the problem?

[smol-ninja]

I have finally managed to modify Synthetix reward contract for Sablier NFTs. I have added only stake and unstake tests for now and other tests are wip.

the individual doing this would essentially be rewarding the stakers with funds that belong to them by the end of the staking campaign. For example, assuming two people have an ongoing stream of the same amount, where one stream has an endTime < stakingEndTime and the other has an endTime > stakingEndTime, it wouldn’t be fair for the person whose stream ends before the staking campaign finishes to generate the same rewards as the other.

I think the objective is to allow stakers to earn rewards on the "total" vesting tokens irrespective of the endTime of the streams. So with that in mind, I think it's not unfair. Again, it's a user specific decision so we can leave it to them to implement

We could modify the mapping as follows

As discussed privately, we can leave it as it is i.e. allowing either lockup linear or lockup dynamic at a time.

[smol-ninja]

@andreivladbrg After the recent conversation with Fjord, I've changed my mind about staking cancelable streams. We should definitely provide a template and a doc to inform them about handling cancelable streams if they get cancelled. I believe our role is to provide as much information as we can to make sure users don't get rugged because of the composability of the Sablier NFTs. And since we cannot stop them from implementing staking for cancelable streams. They will do it if it's their business requirement.

As I mentioned on Slack, the risk involved with the cancelable stream can be mitigated by implementing the onStreamCanceled hook that unstakes the NFT automatically upon cancellation.

Curious what you think about it now? Has the recent conversation with Fjord changed your mind too?

tagging @PaulRBerg and @razgraf for your thoughts as well.

The comment by Fjord below supports my argument:

[PaulRBerg]

And since we cannot stop them from implementing staking for cancelable streams. They will do it if it's their business requirement.

Good point.

risk involved with the cancelable stream can be mitigated by implementing the onStreamCanceled hook

True. But in this case, we should add a BIG warning in the contract that the hook must be implemented.

Has the recent conversation with Fjord changed your mind too?

Yes. Staking has become a more important feature given Fjord's interest in it.

Happy for you and @andreivladbrg to spend more time on this, and to even get it audited.

[PaulRBerg]

Feedback: you should move these conversations into a separate discussion as it appears that the spec of the staking contracts is up for debate. PR comments should be for concrete implementation details.

[andreivladbrg]

Feedback: you should move these conversations into a separate discussion as it appears that the spec of the staking contracts is up for debate. PR comments should be for concrete implementation details.

thanks for the feedback, I believe that this PR is already spammed with comments, creating a new discussion just to brainstorm the implementation would slow down the proces as you will need to switch between tabs and get lost between the replies, wdyt @smol-ninja ?

regarding "spec of the staking contracts is up for debate" - yes, this is a consequence of having an actual implementation which could've been predicted in a pre PR discussion

[PaulRBerg]

disagree but won't debate any further, feel free to do as you wish

[andreivladbrg]

I think the objective is to allow stakers to earn rewards on the "total" vesting tokens irrespective of the endTime of the streams

IMO, the objective is also to provide a realistic scenario. It's not fair to get rewards for tokens that are not yours by the end of the staking campaign, especially for one-time campaigns that are not updated (it's not the case with our current design - Synthetix). Given the design which was implemented, I have changed my mind about this stream.endTime requirement. For non-cancelable streams it is ok to ignore the stream.endTime.

Also, you have not answered this:

Another option that came to mind is to implement a custom withdrawableAmountOf function that takes a time variable as a parameter, which would be the stakingEndTime, to calculate how much one would have by the end of the campaign.

---

I believe our role is to provide as much information as we can to make sure users don't get rugged because of the composability of the Sablier NFTs.

Agree, we should provide as much information, but it doesn't mean we should implement it in the template contract.
At the moment, with the current method of calculatating the amount staked for cancelable streams I don't think is the correct decision to make: to provide it as an example. (it doesn't mean that we can't mention the onStreamCanceled hook)

Why:

One is staking an NFT, calculates the amount staked (let's call it AS) at t0 with amount.deposit - amount.withdrawn - amount.refunded (RF_t0), time passes (t1) and he claims the rewards (X amount). Immediately after this claim action, the sender cancels the stream and gets back the refundable funds (RF_t1), and it means that the NFT owner has claimed/generated at t1, AS - RF_t1 more funds with his NFT.

For cancelable streams, the assets belonged by the sender or recipient are time dependent.

---

Having said that, what about the new way of calculating the underlying amount of assets:

Non-cancelable streams: amount.deposit - amount.withdrawn - amount.refunded

Cancelable streams: just the withdrawable amount, and a new function just to use the updateReward modifier for these kind of NFTs staked. Yes, it wouldn't be the best UX, but it is more realistic and safe.

---

See below. code review (have not looked at tests)

[smol-ninja]

Now we have another customer who asked for our help in the development of the staking contract. So, I have updated this PR to reflect v2.2 changes. @andreivladbrg may I request you to review?

To move fast, I'd want you to keep the following points in mind:

1. This is just a template. So less coverage is fine imo. I expect the integrators to write a complete test and get it audited.
2. Every integrator will have his own requirements so we cannot provide a template for all options. Thus, this template is expected to be used as a skeleton to understand the basic logic.
3. Lax tests are fine as again this is a template.

[andreivladbrg]

• This is just a template. So less coverage is fine imo. I expect the integrators to write a complete test and get it audited.
• Every integrator will have his own requirements so we cannot provide a template for all options. Thus, this template is expected to be used as a skeleton to understand the basic logic.
• Lax tests are fine as again this is a template

agree with your points, i will approve the PR for now to move faster

[smol-ninja]

Thank you.