Production ready? :)

[tempe-techie]

@CjHare Very good idea, this is probably the first ERC-4626 vault implementation where the reward token is different from the underlying token!

Is it production ready yet, or are there still things to be added? 🙂

[CjHare]

Hi @tempe-techie, thanks for taking the time to check the my simple vault 🙂

Having a reward token distinct from the Vault token is a simple change, but it provides many more options in how you can use it e.g approaches used by Sushi swap when they manage their LP pools or Curve using their own tokens for rewards.

Is this production ready, really that depends of your definition for production ready 😉 For me, there are three parts: fixes, features and process.

Auditing by a proper auditor is also worthwhile, after it's considered as production ready.

Fixes In terms of code changes it's removing the '+1" from the preview https://github.com/CjHare/Simple-Rewarding-Vault/blob/5dcbefd03fa7fd894052b59daf58b36901f27566/contracts/vaults/SimpleRewardVault.sol#L71 That was in there to have the test exactly match some requirements I was working from, however I suspect the precise timings of events in blocks were off.

Apart from the above change, the code will run and doesn't suffer from any reentrancy issues or other flaws (as the contract is rather simple).

Features Reward configuration; rewards are currently hardcoded at 1 token a block 10**rewards_.decimals() Improvement here would be to have it as constructor input and beyond that to have a function to change the rewards (this would also require Ownable with onlyOwner modifier).

Pausabilit; the ability to halt and resume interactions (deposits, withdraw, harvest) is nice feature, also needs the concept of Ownable for guarding.

Emergency withdrawal; providing a way for the users to withdraw funds without the Vault having sufficient reward tokens, covers the case of rewards being insufficiently funded. The open question is what happens to the user's rewards in this case, are they fore-fitted or is an additional ledger needed?

Additional input guards; a few more require statements to guard against bad input e.g. address(0) might be useful, but most of those cases get caught eventually due to using SafeERC20, it'd safe the user gas and provide a more meaningful revert message.

Process This repo was created fresh as a minimal dependency set, meaning there's no static analysis hooked up to CI/CD.

CI/CD: GitHub actions to run the various tests.

Static analysis; tool choices would be Slither, SolHint and maybe the MythX webhook service (the first two cover Solidity, while MythX the bytecode),

[CjHare]

I suppose, there's also the part of actually using the staked assets for something, other than just holding them e.g. generating that yield 😆

[tempe-techie]

Hey @CjHare, thanks for a detailed reply.

Yes, the Ownable function would be very beneficial. Or if you're not sure whether to add it or not, you could make two versions instead. One with Ownable (+ all the additional function such as changing rewards, pausability, and owner's emergency withdrawal), and another one more basic without ownership.

About user's emergency withdrawal and this question:

The open question is what happens to the user's rewards in this case, are they fore-fitted or is an additional ledger needed?

I imagine that if reward tokens run out, then the reward period is over. This should be considered as a normal behaviour, not a bug. So I'd say no additional ledger is needed.

What do you think? :)

a few more require statements to guard against bad input e.g. address(0) might be useful, but most of those cases get caught eventually due to using SafeERC20, it'd safe the user gas and provide a more meaningful revert message.

This would save gas in case of a failed tx (the 0 address), but in case of a successful tx (which is the majority of txs) it would add to the gas costs. So perhaps it's not really needed?

[CjHare]

Yes, the Ownable function would be very beneficial. Or if you're not sure whether to add it or not, you could make two versions instead.

That is a good idea, as introducing Ownable does bring with trust assumptions e.g. bait & switch for rewards (with updatable reward address), or freezing assets (Pausability over deposits, withdraws) ...which may not be for everybody.

I imagine that if reward tokens run out, then the reward period is over. This should be considered as a normal behaviour, not a bug. So I'd say no additional ledger is needed.

Ah, there is another scenario to consider! ...when you're able to begin the staking period before rewards have been transferred to the vault (as happens with Sushi / MasterChef and the Simple-Rewarding-Vault) and that transfer has a manual dependency (EOA or external trigger to aa contract e.g multisig), it could be forgotten about, which presents the staker with the option of either emergency withdrawing (loosing their rewards) or waiting until the rewards are given to the vault so they withdraw with rewards. I'm aware of a few cases in the past where Sushi were late with rewards to their LP pools and the users (quite rightly) kicked up a stink on all media platforms ...as they wanted to move their funds to better opportunities and not wait around.

...but that being said, I think emergency withdraw forfeiting rewards is sensible, simpler & gas effective approach.

This would save gas in case of a failed tx (the 0 address), but in case of a successful tx (which is the majority of txs) it would add to the gas costs. So perhaps it's not really needed?

True, as long as the bad input can't cause any problems until it's handled downstream, then adding an additional burden for all flows for that single edge case is sub-optimal 😉

[tempe-techie]

Great! Let me know if you need any help with that.

If so, I think best would be to make a todo list (maybe in the Issues section?) with everything broken down into smaller tasks. Including tests, especially for edge cases.

[CjHare]

For a project this small, GitHub issues are the best fit (GitHub project, or Linear etc would be overkill).

The changes are pretty straightforward (I can easily handle them), if you want to create Issues for the edge cases that are of interest to you, I can cover them ...eventually 😉

For my own curiosity, what you're interest / use case? (unless it's top secret, of course 😁)

[tempe-techie]

Not a secret 😄 I'm a core dev at two projects (@punk-domains and @iggy-social) where tokens are and will be used in various ways. Eventually a staking smart contract will be needed. I was looking into the Synthetix StakingRewards.sol contract which is a de facto industry standard, but it was made way before EIP-4626 so it's not compatible with that standard.

That's why I was searching if there's already an ERC-4626 compliant staking contract for different tokens out there and I found your repo. Perhaps it will become the next de facto standard contract 😉

[CjHare]

Those projects seem cool 🙂

EIP-4626 does add an overhead to token operations, which stems from the abstraction of using shares. The idea of shares is useful for cases where the underlying pool of assets are intended to change due to yield (ideally grow ....but, you never know 🤷 ), however when the intention is the pool of assets will only going to change due to deposits (staking) and withdrawal, you don't get any real value from the additional overhead.

Recently I was looking at Lido staking, their stETH and wstETH, they are a perfect candidates for a EIP-4626 Vault (as the code looks pretty similar to the OZ impl, but they were created before the spec). wstETH being the share and stETH the underlying asset with the Lido staking contract as the vault (validator rewards being paid back into the vault as stETH, expanding the pool of assets that the shares have claim on).

[CjHare]

btw, I'll add some issues for your visibility into the progress (and it's a good habit for me to get back into 😁 )

[tempe-techie]

EIP-4626 does add an overhead to token operations, which stems from the abstraction of using shares. The idea of shares is useful for cases where the underlying pool of assets are intended to change due to yield (ideally grow ....but, you never know 🤷 ), however when the intention is the pool of assets will only going to change due to deposits (staking) and withdrawal, you don't get any real value from the additional overhead.

Yeah I understand, but the part I'm most interested in is the fact that your stake gets tokenized. Which means that you can then sell it (partially or fully), transfer it to another address, or use it in other DeFi protocols (e.g. as collateral in lending protocols) 🙂

[CjHare]

Yeah I understand, but the part I'm most interested in is the fact that your stake gets tokenized. Which means that you can then sell it (partially or fully), transfer it to another address, or use it in other DeFi protocols (e.g. as collateral in lending protocols) 🙂

Fair.

[tempe-techie]

Hey @CjHare, I see that an ERC-4626 alliance was created: https://twitter.com/erc4626/status/1641062968317075461

And also a website with a directory of ERC-4626 implementations: https://erc4626.info/resources/

Would be great to see your repo on this list 😎 Here's a submit form: https://docs.google.com/forms/d/e/1FAIpQLSeOHj98x92-tH23DlYMrSeZwXiq_ES4OCQNeWy1H_WzHd1MIw/viewform

[CjHare]

@tempe-techie thanks for the head's up about the resource, I'll submit via the form after the repo rename ticket 👍