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.

Sign up for GitHub

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

Add a variant of the Vesting wallet for updating the beneficiary #264

Closed
wants to merge 5 commits into from
+159 −0
Closed

Add a variant of the Vesting wallet for updating the beneficiary #264

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5f3cef7
add a variant of the Vesting wallet for updating the beneficiary
Amxx May 9, 2024
0e49e38
up
Amxx May 9, 2024
2bc8040
Update contracts/vesting/VestingWalletRecovery.sol
Amxx May 9, 2024
ea128e2
transitory upgrade
Amxx May 13, 2024
42adfd9
up
Amxx May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions contracts/vesting/VestingWalletRecovery.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// SPDX-License-Identifier: UNLICENSED
// See Forta Network License: https://github.com/forta-network/forta-contracts/blob/master/LICENSE.md

pragma solidity ^0.8.9;

import "./VestingWalletV1.sol";

/**
* This contract is desigend for recovering the in case the beneficiary was lost.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
*/
contract VestingWalletRecovery is VestingWalletV1 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is also VestingWalletV2. I don't know if any of the affected wallets are using V2 but if so this recovery upgrade would remove V2 features.

This seems like another reason to use the "light" recovery mode. It keeps the wallet in the same version it's at (V1 or V2).

event BeneficiaryUpdate(address newBeneficiary);

function updateBeneficiary(address newBeneficiary) external onlyOwner {
_setBeneficiary(newBeneficiary);
emit BeneficiaryUpdate(newBeneficiary);
Copy link

Choose a reason for hiding this comment

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

I'd say this event should be emitted inside of _setBeneficiary instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to minimize the changes to V1. If the storage was not private I wouldn't have touched it.

Copy link

@ernestognw ernestognw May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see. I was thinking of overriding _setBeneficiary in V2:

function _setBeneficiary(address newBeneficiary) override ... {
  super._setBeneficiary(newBeneficiary);
  emit BeneficiaryUpdate(newBeneficiary);
}

Would you say this is acceptable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

feels a bit too much to me. It means having two function instead of one in VestingWalletRecovery.

contract VestingWalletRecovery is VestingWalletV1 {
    event BeneficiaryUpdate(address newBeneficiary);

    function updateBeneficiary(address newBeneficiary) external onlyOwner {
        _setBeneficiary(newBeneficiary);
    }

    function _setBeneficiary(address newBeneficiary) internal virtual override {
        super._setBeneficiary(newBeneficiary);
        emit BeneficiaryUpdate(newBeneficiary);
    }
}

vs

contract VestingWalletRecovery is VestingWalletV1 {
    event BeneficiaryUpdate(address newBeneficiary);

    function updateBeneficiary(address newBeneficiary) external onlyOwner {
        _setBeneficiary(newBeneficiary);
        emit BeneficiaryUpdate(newBeneficiary);
    }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with prioritizing fewer code and fewer changes to V1.

}
Copy link

Choose a reason for hiding this comment

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

Would it make sense to add another function to accept the beneficiary change? That might be a way of avoiding an issue of changing the beneficiary to a centralized exchange wallet (as an example)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That might be a way of avoiding an issue of changing the beneficiary to a centralized exchange wallet

My understanding is that is exactly what they plan to use. Apparently, coinbase has a thing where the user as a "long lasting" address that can receive assets (including ERC20) ... but I'm not 100% sure it can be use to interract with arbitrary smart contracts.

Copy link

@ernestognw ernestognw May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right, I recall someone mention the Coinbase Account, but if that's the purpose then it is okay under the assumption that this VestingWallet is more legally-restricted than code-restricted. Let's just keep this in mind, might be an important detail before making the change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah since release is permisionless we can't assume that the beneficiary wallet is able to interact with the smart contract.

}
4 changes: 4 additions & 0 deletions contracts/vesting/VestingWalletV1.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ contract VestingWalletV1 is OwnableUpgradeable, UUPSUpgradeable {
return _released[token];
}

function _setBeneficiary(address newBeneficiary) internal {
_beneficiary = newBeneficiary;
}

/**
* @dev Release the tokens that have vested by the specified timestamp.
*/
Expand Down
65 changes: 65 additions & 0 deletions test/vesting/VestingWallet.recovery.test.js
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

Following Fran's comments, I'd rename this file to VestingWalletV1.recovery.test.js. Consistent with the name of the underlying wallet that's being updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#264 (comment)

Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
const hre = require('hardhat');
const { ethers } = hre;
const { expect } = require('chai');
const { prepare, deployUpgradeable, performUpgrade, deploy, attach } = require('../fixture');
const utils = require('../../scripts/utils');

const allocation = {
start: utils.dateToTimestamp('2021-09-01T00:00:00Z'),
cliff: utils.durationToSeconds('1 year'),
duration: utils.durationToSeconds('4 years'),
};

describe('VestingWallet ', function () {
prepare();

describe('Vesting recovery', function () {
describe('vesting with admin', function () {
beforeEach(async function () {
allocation.beneficiary = this.accounts.user1.address;
allocation.owner = this.accounts.admin.address;

this.vesting = await deployUpgradeable(
hre,
'VestingWallet',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be deploying "V0" of the vesting wallet:

forta-contracts/contracts/vesting/_old/vesting_wallet/VestingWalletV0.sol

Line 12 in 7d5489a

contract VestingWallet is OwnableUpgradeable, UUPSUpgradeable {

I think it should be VestingWalletV2 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The wallets we are targeting are using V0.

Not that the storage layout described in VestingWalletRecoveryLight is common to V0, V1 and V2.

'uups',
[allocation.beneficiary, allocation.owner, allocation.start, allocation.cliff, allocation.duration],
{ unsafeAllow: 'delegatecall' }
);
await Promise.all([this.vesting.start(), this.vesting.cliff(), this.vesting.duration(), this.vesting.beneficiary(), this.vesting.owner()]).then(
([start, cliff, duration, beneficiary, owner]) => {
expect(start).to.be.equal(allocation.start);
expect(cliff).to.be.equal(allocation.cliff);
expect(duration).to.be.equal(allocation.duration);
expect(beneficiary).to.be.equal(allocation.beneficiary);
expect(owner).to.be.equal(allocation.owner);
}
);
});

it('perform recovery', async function () {
this.vesting = await performUpgrade(hre, this.vesting, 'VestingWalletRecovery', {
unsafeAllow: 'delegatecall',
});

// restricted
await expect(this.vesting.connect(this.accounts.other).updateBeneficiary(this.accounts.other.address))
.to.be.revertedWith(`Ownable: caller is not the owner`);

// authorized
await expect(this.vesting.connect(this.accounts.admin).updateBeneficiary(this.accounts.user2.address))
.to.emit(this.vesting, 'BeneficiaryUpdate').withArgs(this.accounts.user2.address);

await Promise.all([this.vesting.start(), this.vesting.cliff(), this.vesting.duration(), this.vesting.beneficiary(), this.vesting.owner()]).then(
([start, cliff, duration, beneficiary, owner]) => {
expect(start).to.be.equal(allocation.start);
expect(cliff).to.be.equal(allocation.cliff);
expect(duration).to.be.equal(allocation.duration);
expect(beneficiary).to.be.equal(this.accounts.user2.address);
expect(owner).to.be.equal(allocation.owner);
}
);
});
});
});
});
Loading