Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Ownable in VestingWallet instead of an immutable beneficiary #4508

Merged
merged 10 commits into from
Aug 4, 2023

Conversation

ernestognw
Copy link
Member

Fixes LIB-996
Fixes LIB-986

Currently, the beneficiary of the VestingWallet is immutable without an option for transferability. This is solved by making the VestingWallet inherit from Ownable2Step, also allowing to restrict the release() function to only the interested party.

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@ernestognw ernestognw requested a review from frangio August 4, 2023 04:54
@changeset-bot
Copy link

changeset-bot bot commented Aug 4, 2023

🦋 Changeset detected

Latest commit: 4ce8164

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ernestognw ernestognw requested a review from Amxx August 4, 2023 04:56
*/
constructor(address beneficiaryAddress, uint64 startTimestamp, uint64 durationSeconds) payable {
constructor(address beneficiaryAddress, uint64 startTimestamp, uint64 durationSeconds) payable Ownable(msg.sender) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to discuss doing this:

Suggested change
constructor(address beneficiaryAddress, uint64 startTimestamp, uint64 durationSeconds) payable Ownable(msg.sender) {
constructor(address benefactorAddress, address beneficiaryAddress, uint64 startTimestamp, uint64 durationSeconds) payable Ownable(benefactorAddress) {

I think it has a few extra implications we need to consider, but I think this is definitely the way to go because a VestingWallet is the kind of contract that might be deployed with immutable clones. In such cases the deployer won't be able to claim the tokens back in case they're never claimed.

I guess the best solution to this is to allow setting a benefactor address (perhaps with a different name)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can present it as a "recovery address" in case the beneficiary doesn't claim ownership of the vesting

Suggested change
constructor(address beneficiaryAddress, uint64 startTimestamp, uint64 durationSeconds) payable Ownable(msg.sender) {
constructor(address beneficiaryAddress, address recoveryAddress, uint64 startTimestamp, uint64 durationSeconds) payable Ownable(recoveryAddress) {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is "claiming owership of the vesting a taxable event"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is, because a user claiming the ownership doesn't have access to transfer the tokens yet.

Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an interresting approach, but a very opinionated one. I don't feel confortable doing that change for our main VestingWallet contract.

Basically, this feels like a preset. Something interresting that is built on top of our building blocks. Something that we may want to provide as a "ready to use to address interresting usecases" but not as "the defaut vesting wallet building block"


What I would do:

  • VestingWallet:

    • abstract (so it can't be deployed like this)
    • does not have any beneficiary in storage (no constructor argument for it)
    • includes an abstract function beneficiary() public view return (address); unimplemented function. (this forces the abstract part)
    • no protection on the public release functions.
  • A new contract. (where? contracts/finance/preset, contracts/finance/shelve, contracts/shelve ???)

contract VestingWalletClaimable is VestingWallet, Ownable2Step {
    constructor VestingWalletClaimable(address beneficiaryAddress, address recoveryAddress, uint64 startTimestamp, uint64 durationSeconds) 
        payable 
        VestingWallet(startTimestamp, durationSeconds) 
        Ownable(recoveryAddress) 
    {
        transferOwnership(beneficiaryAddress);
    }
    
    function beneficiary() public view virtual override returns (address) {
        return owner();
    }
    
    function release() public virtual override onlyOwner {
        super.release();
    }
    
    function release(address token) public virtual override onlyOwner {
        super.release(token);
    }
}

constructor(address beneficiaryAddress, uint64 startTimestamp, uint64 durationSeconds) payable Ownable(msg.sender) {
if (beneficiaryAddress == address(0)) {
constructor(address beneficiary, uint64 startTimestamp, uint64 durationSeconds) payable Ownable(beneficiary) {
if (beneficiary == address(0)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe not in the PR, but we should remove that check here and move it to Ownable's constructor

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we should reconsider this. I opened an issue
#4511

test/utils/Create2.test.js Outdated Show resolved Hide resolved
@ernestognw
Copy link
Member Author

Yes, I'm also not in favor of this change. We prepared it as a response for the audit so we can evaluate it.

My opinion is also to not make this change because it's very easy to implement by inheritance and it's also forcing users into a behavior.

Playing devil's advocate, I really like the design of a pending owner that needs to claim the VestingWallet. The concept of recovering may be interesting to explore, but doesn't justify adding Ownable2Step imo.

contracts/finance/VestingWallet.sol Outdated Show resolved Hide resolved
constructor(address beneficiaryAddress, uint64 startTimestamp, uint64 durationSeconds) payable Ownable(msg.sender) {
if (beneficiaryAddress == address(0)) {
constructor(address beneficiary, uint64 startTimestamp, uint64 durationSeconds) payable Ownable(beneficiary) {
if (beneficiary == address(0)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we should reconsider this. I opened an issue
#4511

@ernestognw ernestognw changed the title Use Ownable2Step in VestingWallet with restricted release() Use Ownable in VestingWallet instead of an immutable beneficiary Aug 4, 2023
Copy link
Member Author

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me again

This was referenced Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants