Use Ownable in VestingWallet instead of an immutable beneficiary (#4508)

Co-authored-by: Francisco Giordano <[email protected]>

Author: ernestognw

.changeset/healthy-gorillas-applaud.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`VestingWallet`: Use `Ownable` instead of an immutable `beneficiary`.

contracts/finance/VestingWallet.sol

@@ -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);
 
@@ -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)) {
             revert VestingWalletInvalidBeneficiary(address(0));
         }
-        _beneficiary = beneficiaryAddress;
+
         _start = startTimestamp;
         _duration = durationSeconds;
     }
@@ -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.
      */
@@ -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);
     }
 
     /**
@@ -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);
     }
 
     /**

test/finance/VestingWallet.test.js

@@ -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));

test/utils/Create2.test.js

@@ -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 () {