Use Ownable in VestingWallet instead of an immutable beneficiary

contracts/finance/VestingWallet.sol

@@ -6,26 +6,25 @@ 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 {Ownable2Step, Ownable} from "../access/Ownable2Step.sol";
+import {Ownable} from "../access/Ownable.sol";
 
 /**
- * @title VestingWallet
- * @dev Handles the vesting of native currency and ERC20 tokens for a given beneficiary who gets the ownership of the
- * contract by calling {Ownable2Step-acceptOwnership} to accept a 2-step ownership transfer setup at deployment with
- * {Ownable2Step-transferOwnership}. The initial owner of this contract is the benefactor (`msg.sender`), enabling them
- * to recover any unclaimed tokens.
+ * @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.
  *
- * Custody of multiple tokens can be given to this contract, which will release the tokens to the beneficiary following
- * a given vesting schedule that is customizable through the {vestedAmount} function.
- *
- * 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.
  */
-contract VestingWallet is Context, Ownable2Step {
+contract VestingWallet is Context, Ownable {
     event EtherReleased(uint256 amount);
     event ERC20Released(address indexed token, uint256 amount);
 
@@ -43,11 +42,10 @@ contract VestingWallet is Context, Ownable2Step {
      * @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 Ownable(msg.sender) {
-        if (beneficiaryAddress == address(0)) {
+    constructor(address beneficiary, uint64 startTimestamp, uint64 durationSeconds) payable Ownable(beneficiary) {
+        if (beneficiary == address(0)) {
             revert VestingWalletInvalidBeneficiary(address(0));
         }
-        transferOwnership(beneficiaryAddress);
 
         _start = startTimestamp;
         _duration = durationSeconds;
@@ -113,7 +111,7 @@ contract VestingWallet is Context, Ownable2Step {
      *
      * Emits a {EtherReleased} event.
      */
-    function release() public virtual onlyOwner {
+    function release() public virtual {
         uint256 amount = releasable();
         _released += amount;
         emit EtherReleased(amount);
@@ -125,7 +123,7 @@ contract VestingWallet is Context, Ownable2Step {
      *
      * Emits a {ERC20Released} event.
      */
-    function release(address token) public virtual onlyOwner {
+    function release(address token) public virtual {
         uint256 amount = releasable(token);
         _erc20Released[token] += amount;
         emit ERC20Released(token, amount);

test/finance/VestingWallet.behavior.js

@@ -1,14 +1,12 @@
-const { time, setNextBlockBaseFeePerGas } = require('@nomicfoundation/hardhat-network-helpers');
+const { time } = require('@nomicfoundation/hardhat-network-helpers');
 const { expectEvent } = require('@openzeppelin/test-helpers');
 const { expect } = require('chai');
-const { web3 } = require('hardhat');
-const { expectRevertCustomError } = require('../helpers/customError');
 
 function releasedEvent(token, amount) {
   return token ? ['ERC20Released', { token: token.address, amount }] : ['EtherReleased', { amount }];
 }
 
-function shouldBehaveLikeVesting(beneficiary, accounts) {
+function shouldBehaveLikeVesting(beneficiary) {
   it('check vesting schedule', async function () {
     const [vestedAmount, releasable, ...args] = this.token
       ? ['vestedAmount(address,uint64)', 'releasable(address)', this.token.address]
@@ -24,51 +22,35 @@ function shouldBehaveLikeVesting(beneficiary, accounts) {
     }
   });
 
-  describe('execute vesting schedule', async function () {
-    beforeEach(async function () {
-      [this.release, ...this.args] = this.token ? ['release(address)', this.token.address] : ['release()'];
-    });
+  it('execute vesting schedule', async function () {
+    const [release, ...args] = this.token ? ['release(address)', this.token.address] : ['release()'];
 
-    it('releases a linearly vested schedule', async function () {
-      let released = web3.utils.toBN(0);
-      const before = await this.getBalance(beneficiary);
-      const releaser = await this.mock.owner();
+    let released = web3.utils.toBN(0);
+    const before = await this.getBalance(beneficiary);
 
-      {
-        // Allows gas price to be 0 so no ETH is spent in the transaction.
-        await setNextBlockBaseFeePerGas(0);
+    {
+      const receipt = await this.mock.methods[release](...args);
 
-        const receipt = await this.mock.methods[this.release](...this.args, { from: releaser, gasPrice: 0 });
-        await expectEvent.inTransaction(receipt.tx, this.mock, ...releasedEvent(this.token, '0'));
-        await this.checkRelease(receipt, beneficiary, '0');
+      await expectEvent.inTransaction(receipt.tx, this.mock, ...releasedEvent(this.token, '0'));
 
-        expect(await this.getBalance(beneficiary)).to.be.bignumber.equal(before);
-      }
+      await this.checkRelease(receipt, beneficiary, '0');
 
-      for (const timestamp of this.schedule) {
-        await time.setNextBlockTimestamp(timestamp);
-        const vested = this.vestingFn(timestamp);
+      expect(await this.getBalance(beneficiary)).to.be.bignumber.equal(before);
+    }
 
-        // Allows gas price to be 0 so no ETH is spent in the transaction.
-        await setNextBlockBaseFeePerGas(0);
+    for (const timestamp of this.schedule) {
+      await time.setNextBlockTimestamp(timestamp);
+      const vested = this.vestingFn(timestamp);
 
-        const receipt = await this.mock.methods[this.release](...this.args, { from: releaser, gasPrice: 0 });
-        await expectEvent.inTransaction(receipt.tx, this.mock, ...releasedEvent(this.token, vested.sub(released)));
-        await this.checkRelease(receipt, beneficiary, vested.sub(released));
+      const receipt = await this.mock.methods[release](...args);
+      await expectEvent.inTransaction(receipt.tx, this.mock, ...releasedEvent(this.token, vested.sub(released)));
 
-        expect(await this.getBalance(beneficiary)).to.be.bignumber.equal(before.add(vested));
+      await this.checkRelease(receipt, beneficiary, vested.sub(released));
 
-        released = vested;
-      }
-    });
+      expect(await this.getBalance(beneficiary)).to.be.bignumber.equal(before.add(vested));
 
-    it('cannot be released by a non releaser', async function () {
-      await expectRevertCustomError(
-        this.mock.methods[this.release](...this.args, { from: accounts[0] }),
-        'OwnableUnauthorizedAccount',
-        [accounts[0]],
-      );
-    });
+      released = vested;
+    }
   });
 }
 

test/finance/VestingWallet.test.js

@@ -18,7 +18,6 @@ contract('VestingWallet', function (accounts) {
   beforeEach(async function () {
     this.start = (await time.latest()).addn(3600); // in 1 hour
     this.mock = await VestingWallet.new(beneficiary, this.start, duration);
-    await this.mock.acceptOwnership({ from: beneficiary });
   });
 
   it('rejects zero address for beneficiary', async function () {
@@ -29,12 +28,6 @@ contract('VestingWallet', function (accounts) {
     );
   });
 
-  it('sets the initial owner as the sender (benefactor)', async function () {
-    this.mock = await VestingWallet.new(beneficiary, this.start, duration, { from: sender });
-    expect(await this.mock.owner()).to.be.equal(sender);
-    expect(await this.mock.pendingOwner()).to.be.equal(beneficiary);
-  });
-
   it('check vesting contract', async function () {
     expect(await this.mock.owner()).to.be.equal(beneficiary);
     expect(await this.mock.start()).to.be.bignumber.equal(this.start);
@@ -57,7 +50,7 @@ contract('VestingWallet', function (accounts) {
         this.checkRelease = () => {};
       });
 
-      shouldBehaveLikeVesting(beneficiary, accounts);
+      shouldBehaveLikeVesting(beneficiary);
     });
 
     describe('ERC20 vesting', function () {
@@ -70,7 +63,7 @@ contract('VestingWallet', function (accounts) {
         await this.token.$_mint(this.mock.address, amount);
       });
 
-      shouldBehaveLikeVesting(beneficiary, accounts);
+      shouldBehaveLikeVesting(beneficiary);
     });
   });
 });