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
5 changes: 5 additions & 0 deletions .changeset/healthy-gorillas-applaud.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': major
---

`VestingWallet`: Use `Ownable` instead of an immutable `beneficiary`.
37 changes: 17 additions & 20 deletions contracts/finance/VestingWallet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,28 @@ import {IERC20} from "../token/ERC20/IERC20.sol";
import {SafeERC20} from "../token/ERC20/utils/SafeERC20.sol";
import {Address} from "../utils/Address.sol";
import {Context} from "../utils/Context.sol";
import {Ownable} from "../access/Ownable.sol";

/**
* @title VestingWallet
* @dev This contract handles the vesting of Eth and ERC20 tokens for a given beneficiary. Custody of multiple tokens
* can be given to this contract, which will release the token to the beneficiary following a given vesting schedule.
* The vesting schedule is customizable through the {vestedAmount} function.
* @dev A vesting wallet is an ownable contract that can receive native currency and ERC20 tokens, and release these
* assets to the wallet owner, also referred to as "beneficiary", according to a vesting schedule.
*
* Any token transferred to this contract will follow the vesting schedule as if they were locked from the beginning.
* Any assets transferred to this contract will follow the vesting schedule as if they were locked from the beginning.
* Consequently, if the vesting has already started, any amount of tokens sent to this contract will (at least partly)
* be immediately releasable.
*
* By setting the duration to 0, one can configure this contract to behave like an asset timelock that hold tokens for
* a beneficiary until a specified time.
*
* NOTE: Since the wallet is {Ownable}, and ownership can be transferred, it is possible to sell unvested tokens.
* Preventing this in a smart contract is difficult, considering that: 1) a beneficiary address could be a
* counterfactually deployed contract, 2) there is likely to be a migration path for EOAs to become contracts in the
* near future.
*
* NOTE: When using this contract with any token whose balance is adjusted automatically (i.e. a rebase token), make sure
* to account the supply/balance adjustment in the vesting schedule to ensure the vested amount is as intended.
*/
contract VestingWallet is Context {
contract VestingWallet is Context, Ownable {
event EtherReleased(uint256 amount);
event ERC20Released(address indexed token, uint256 amount);

Expand All @@ -34,18 +38,18 @@ contract VestingWallet is Context {

uint256 private _released;
mapping(address => uint256) private _erc20Released;
address private immutable _beneficiary;
uint64 private immutable _start;
uint64 private immutable _duration;

/**
* @dev Set the beneficiary, start timestamp and vesting duration of the vesting wallet.
* @dev Sets the sender as the initial owner, the beneficiary as the pending owner, the start timestamp and the
* vesting duration of the vesting wallet.
*/
constructor(address beneficiaryAddress, uint64 startTimestamp, uint64 durationSeconds) payable {
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

revert VestingWalletInvalidBeneficiary(address(0));
}
_beneficiary = beneficiaryAddress;

_start = startTimestamp;
_duration = durationSeconds;
}
Expand All @@ -55,13 +59,6 @@ contract VestingWallet is Context {
*/
receive() external payable virtual {}

/**
* @dev Getter for the beneficiary address.
*/
function beneficiary() public view virtual returns (address) {
return _beneficiary;
}

/**
* @dev Getter for the start timestamp.
*/
Expand Down Expand Up @@ -121,7 +118,7 @@ contract VestingWallet is Context {
uint256 amount = releasable();
_released += amount;
emit EtherReleased(amount);
Address.sendValue(payable(beneficiary()), amount);
Address.sendValue(payable(owner()), amount);
}

/**
Expand All @@ -133,7 +130,7 @@ contract VestingWallet is Context {
uint256 amount = releasable(token);
_erc20Released[token] += amount;
emit ERC20Released(token, amount);
SafeERC20.safeTransfer(IERC20(token), beneficiary(), amount);
SafeERC20.safeTransfer(IERC20(token), owner(), amount);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion test/finance/VestingWallet.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ contract('VestingWallet', function (accounts) {
});

it('check vesting contract', async function () {
expect(await this.mock.beneficiary()).to.be.equal(beneficiary);
expect(await this.mock.owner()).to.be.equal(beneficiary);
expect(await this.mock.start()).to.be.bignumber.equal(this.start);
expect(await this.mock.duration()).to.be.bignumber.equal(duration);
expect(await this.mock.end()).to.be.bignumber.equal(this.start.add(duration));
Expand Down
4 changes: 3 additions & 1 deletion test/utils/Create2.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ contract('Create2', function (accounts) {
addr: offChainComputed,
});

expect(await VestingWallet.at(offChainComputed).then(instance => instance.beneficiary())).to.be.equal(other);
const instance = await VestingWallet.at(offChainComputed);

expect(await instance.owner()).to.be.equal(other);
});

it('deploys a contract with funds deposited in the factory', async function () {
Expand Down
Loading