-
Notifications
You must be signed in to change notification settings - Fork 16
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 { | ||
event BeneficiaryUpdate(address newBeneficiary); | ||
|
||
function updateBeneficiary(address newBeneficiary) external onlyOwner { | ||
_setBeneficiary(newBeneficiary); | ||
emit BeneficiaryUpdate(newBeneficiary); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say this event should be emitted inside of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. I was thinking of overriding
Would you say this is acceptable? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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);
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with prioritizing fewer code and fewer changes to V1. |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah since |
||
} |
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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 { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should either be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be "change beneficiary"! Owner is a different entity. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly the name of the function should be |
||||||
_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(); | ||||||
} | ||||||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following Fran's comments, I'd rename this file to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be deploying "V0" of the vesting wallet:
I think it should be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||
'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); | ||||
} | ||||
); | ||||
}); | ||||
}); | ||||
}); | ||||
}); |
There was a problem hiding this comment.
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).