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

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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 designed for recovery in case the beneficiary was lost.
*/
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);

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

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.

}

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

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.

}
60 changes: 60 additions & 0 deletions contracts/vesting/VestingWalletRecoveryLight.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// 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 "@openzeppelin/contracts/utils/Address.sol";
import "@openzeppelin/contracts/utils/StorageSlot.sol";

/**
* This contract is designed for recovery in case the beneficiary was lost.
*/
contract VestingWalletRecoveryLight {
/// Storage
Copy link
Collaborator

@frangio frangio May 13, 2024

Choose a reason for hiding this comment

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

I think that the storage layout should be that of V2.

// Initializable
uint8 private _initialized;
bool private _initializing;
// ContextUpgradeable
uint256[50] private __gap_1;
// OwnableUpgradeable
address private _owner;
uint256[49] private __gap_2;
// UUPSUpgradeable
uint256[50] private __gap_3;
// ERC1967UpgradeUpgradeable
uint256[50] private __gap_4;
// VestingWallerV1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// VestingWallerV1
// VestingWalletV1

mapping (address => uint256) private _released;
address private _beneficiary;
uint256 private _start;
uint256 private _cliff;
uint256 private _duration;

/// Constants and Events
// ERC1967UpgradeUpgradeable
bytes32 internal constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
event Upgraded(address indexed implementation);

function changeOwnerAndUpgrade(address newBeneficiary, address newImplementation) external {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should either be onlyOwner or the parameters should be immutably set in the constructor. Otherwise there is a possible error where the wallet is upgraded without calling this function and it becomes open to anyone to steal the tokens.

Choose a reason for hiding this comment

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

Yeah, considering it's supposed to be light, then setting immutables at construction sounds better

// change ownership
Comment on lines +38 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be "change beneficiary"! Owner is a different entity.

Choose a reason for hiding this comment

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

Similarly the name of the function should be changeBeneficiaryAndUpgrade

_beneficiary = newBeneficiary;

// ERC1967Upgrade._setImplementation
require(Address.isContract(newImplementation), "ERC1967: new implementation is not a contract");
StorageSlot.getAddressSlot(_IMPLEMENTATION_SLOT).value = newImplementation;
emit Upgraded(newImplementation);
}

function proxiableUUID() external pure returns (bytes32) {
return _IMPLEMENTATION_SLOT;
}


function upgradeTo(address) external pure {
revert();
}

function upgradeToAndCall(address, bytes memory) external pure {
revert();
}
}
4 changes: 4 additions & 0 deletions contracts/vesting/VestingWalletV1.sol
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
77 changes: 77 additions & 0 deletions test/vesting/VestingWallet.recovery.test.js

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
const hre = require('hardhat');
const { ethers } = hre;
const { expect } = require('chai');
const { prepare, deployUpgradeable, performUpgrade } = 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.newBeneficiary = this.accounts.user2.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:

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 (full upgrade)', 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(allocation.newBeneficiary))
.to.emit(this.vesting, 'BeneficiaryUpdate').withArgs(allocation.newBeneficiary);
});

it('perform recovery (transitory upgrade)', async function () {
const implementation = await hre.upgrades.erc1967.getImplementationAddress(this.vesting.address);

await performUpgrade(hre, this.vesting, 'VestingWalletRecoveryLight', {
call: { fn: 'changeOwnerAndUpgrade', args: [allocation.newBeneficiary, implementation] },
unsafeAllow: 'delegatecall'
});
});

afterEach(async function () {
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.newBeneficiary);
expect(owner).to.be.equal(allocation.owner);
}
);
});
});
});
});
Loading