Skip to content
Open
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
80 changes: 80 additions & 0 deletions contracts/finance/VestingWalletRevocable.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v5.1.0) (finance/VestingWalletRevocable.sol)

pragma solidity ^0.8.20;

import {VestingWallet} from "./VestingWallet.sol";
import {SafeERC20} from "../token/ERC20/utils/SafeERC20.sol";
import {IERC20} from "../token/ERC20/IERC20.sol";
import {Address} from "../utils/Address.sol";

/**
* @dev Extension of {VestingWallet} that lets the owner revoke the vesting schedule. Unvested ETH and any
* listed ERC20 tokens are returned to the owner, and the vested amount is frozen at the revocation timestamp.
*
* _Available since v5.1._
*/
abstract contract VestingWalletRevocable is VestingWallet {
using SafeERC20 for IERC20;

bool private _revoked;
uint64 private _revokedAt;
uint256 private _ethAllocationSnapshot;
mapping(address token => uint256) private _erc20AllocationSnapshot;

/// @dev The vesting schedule has already been revoked.
error AlreadyRevoked();

/// @dev Emitted when the vesting schedule is revoked.
event VestingRevoked(address indexed owner);

/// @dev Whether the vesting schedule has been revoked.
function isRevoked() public view returns (bool) {
return _revoked;
}

/**
* @dev Revokes the vesting schedule. The unvested portion of ETH and each listed ERC20 token is sent to
* the owner. Tokens omitted from `tokens` stay in the contract and continue to vest against their on-chain
* balance at the revocation timestamp.
*
* Emits a {VestingRevoked} event.
*/
function revoke(address[] calldata tokens) external onlyOwner {
if (_revoked) revert AlreadyRevoked();

uint64 t = uint64(block.timestamp);
_revokedAt = t;
_ethAllocationSnapshot = address(this).balance + released();
for (uint256 i = 0; i < tokens.length; i++) {
_erc20AllocationSnapshot[tokens[i]] = IERC20(tokens[i]).balanceOf(address(this)) + released(tokens[i]);
}
_revoked = true;

uint256 unvestedEth = address(this).balance - _vestingSchedule(_ethAllocationSnapshot, t);
if (unvestedEth > 0) Address.sendValue(payable(owner()), unvestedEth);

for (uint256 i = 0; i < tokens.length; i++) {
uint256 unvested = IERC20(tokens[i]).balanceOf(address(this)) -
_vestingSchedule(_erc20AllocationSnapshot[tokens[i]], t);
if (unvested > 0) IERC20(tokens[i]).safeTransfer(owner(), unvested);
}
Comment on lines +54 to +61
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

Critical: Unvested amount calculation is incorrect when tokens have been released before revocation.

The unvested amount calculation on lines 54 and 58-59 is missing a crucial term. The formulas currently calculate:

unvestedEth = balance - vestedAmount
unvestedTokens = balance - vestedAmount

However, the correct formula should be:

unvestedEth = balance - vestedAmount + released()
unvestedTokens = balance - vestedAmount + released(token)

Why this matters:

The total allocation is balance + released. The unvested portion of this total is totalAllocation - vestedAmount = (balance + released) - vestedAmount. The current code omits the + released term.

Impact when released > 0:

Consider this scenario:

  • Total allocation: 100 tokens
  • At t₁ (25% duration): 25 tokens vest, beneficiary releases all 25
  • At t₂ (50% duration): 50 tokens vested total, owner calls revoke()
  • Balance: 75, Released: 25, Vested: 50

Expected: Owner should receive 100 - 50 = 50 unvested tokens
Actual (buggy): Owner receives 75 - 50 = 25 tokens

The owner loses 25 tokens (equal to the pre-revocation released amount), and the beneficiary can claim those 25 unvested tokens.

Why tests don't catch this: All existing tests call revoke() before any release() calls, so released() = 0 and both formulas coincide.

🔧 Proposed fix
-        uint256 unvestedEth = address(this).balance - _vestingSchedule(_ethAllocationSnapshot, t);
+        uint256 unvestedEth = address(this).balance - _vestingSchedule(_ethAllocationSnapshot, t) + released();
         if (unvestedEth > 0) Address.sendValue(payable(owner()), unvestedEth);
 
         for (uint256 i = 0; i < tokens.length; i++) {
             uint256 unvested = IERC20(tokens[i]).balanceOf(address(this)) -
-                _vestingSchedule(_erc20AllocationSnapshot[tokens[i]], t);
+                _vestingSchedule(_erc20AllocationSnapshot[tokens[i]], t) +
+                released(tokens[i]);
             if (unvested > 0) IERC20(tokens[i]).safeTransfer(owner(), unvested);
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contracts/finance/VestingWalletRevocable.sol` around lines 54 - 61, The
unvested calculation in revoke() is missing already-released amounts so owner
gets wrong funds when released() > 0; update the two computations to treat total
allocation = current balance + released: set unvestedEth = address(this).balance
+ released() - _vestingSchedule(_ethAllocationSnapshot, t) and for tokens set
unvested = IERC20(tokens[i]).balanceOf(address(this)) + released(tokens[i]) -
_vestingSchedule(_erc20AllocationSnapshot[tokens[i]], t); keep the same transfer
calls (Address.sendValue / safeTransfer) and ensure you use the existing
released()/released(token) functions and _vestingSchedule/_*_snapshot
identifiers so underflow cannot occur.


emit VestingRevoked(owner());
}

/// @dev After revocation, returns the vested amount frozen at the revocation timestamp.
function vestedAmount(uint64 timestamp) public view virtual override returns (uint256) {
return _revoked ? _vestingSchedule(_ethAllocationSnapshot, _revokedAt) : super.vestedAmount(timestamp);
}

/**
* @dev After revocation, returns the token's vested amount frozen at the revocation timestamp. Tokens
* omitted from {revoke} keep their on-chain balance and vest against it using the revocation timestamp.
*/
function vestedAmount(address token, uint64 timestamp) public view virtual override returns (uint256) {
if (!_revoked) return super.vestedAmount(token, timestamp);
uint256 snapshot = _erc20AllocationSnapshot[token];
return snapshot == 0 ? super.vestedAmount(token, _revokedAt) : _vestingSchedule(snapshot, _revokedAt);
}
}
109 changes: 109 additions & 0 deletions test/finance/VestingWalletRevocable.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
const { ethers } = require('hardhat');
const { expect } = require('chai');
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');

const { min } = require('../helpers/math');
const time = require('../helpers/time');

const { envSetup, shouldBehaveLikeVesting } = require('./VestingWallet.behavior');

async function fixture() {
const amount = ethers.parseEther('100');
const duration = time.duration.years(4);
const start = (await time.clock.timestamp()) + time.duration.hours(1);

const [sender, beneficiary] = await ethers.getSigners();
const mock = await ethers.deployContract('$VestingWalletRevocable', [beneficiary, start, duration]);

const token = await ethers.deployContract('$ERC20', ['Name', 'Symbol']);
await token.$_mint(mock, amount);
await sender.sendTransaction({ to: mock, value: amount });

const env = await envSetup(mock, beneficiary, token);

const schedule = Array.from({ length: 64 }, (_, i) => (BigInt(i) * duration) / 60n + start);
const vestingFn = timestamp => min(amount, (amount * (timestamp - start)) / duration);

return { mock, duration, start, beneficiary, token, env, schedule, vestingFn };
}

describe('VestingWalletRevocable', function () {
beforeEach(async function () {
Object.assign(this, await loadFixture(fixture));
});

it('check vesting contract', async function () {
expect(await this.mock.owner()).to.equal(this.beneficiary);
expect(await this.mock.start()).to.equal(this.start);
expect(await this.mock.duration()).to.equal(this.duration);
expect(await this.mock.end()).to.equal(this.start + this.duration);
expect(await this.mock.isRevoked()).to.be.false;
});

describe('vesting schedule', function () {
describe('Eth vesting', function () {
beforeEach(async function () {
Object.assign(this, this.env.eth);
});

shouldBehaveLikeVesting();
});

describe('ERC20 vesting', function () {
beforeEach(async function () {
Object.assign(this, this.env.token);
});

shouldBehaveLikeVesting();
});
});

describe('revoke', function () {
it('reverts when caller is not owner', async function () {
const [, , other] = await ethers.getSigners();
await expect(this.mock.connect(other).revoke([]))
.to.be.revertedWithCustomError(this.mock, 'OwnableUnauthorizedAccount')
.withArgs(other.address);
});

it('reverts on second revoke', async function () {
await this.mock.connect(this.beneficiary).revoke([this.token.target]);
await expect(this.mock.connect(this.beneficiary).revoke([this.token.target])).to.be.revertedWithCustomError(
this.mock,
'AlreadyRevoked',
);
});

it('returns unvested funds to owner', async function () {
await time.increaseTo.timestamp(this.start + this.duration / 4n, false);
const tx = await this.mock.connect(this.beneficiary).revoke([this.token.target]);
const unvested = ethers.parseEther('75');

await expect(tx).to.emit(this.mock, 'VestingRevoked').withArgs(this.beneficiary.address);
await expect(tx).to.changeEtherBalances([this.mock, this.beneficiary], [-unvested, unvested]);
await expect(tx).to.changeTokenBalances(this.token, [this.mock, this.beneficiary], [-unvested, unvested]);
expect(await this.mock.isRevoked()).to.be.true;
});
Comment on lines +77 to +86
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add test coverage for revocation after partial release.

The current test verifies revocation works when released = 0, but there's no test for the critical scenario where some amount has been released before calling revoke().

This test gap allowed a critical bug in the contract to go undetected (see the contract review comment about incorrect unvested calculation).

🧪 Suggested additional test case
it('returns correct unvested funds after partial release', async function () {
  // Release at 1/4 duration
  await time.increaseTo.timestamp(this.start + this.duration / 4n, false);
  await this.mock.connect(this.beneficiary).release();
  const released = ethers.parseEther('25');
  
  // Revoke at 1/2 duration
  await time.increaseTo.timestamp(this.start + this.duration / 2n, false);
  const tx = await this.mock.connect(this.beneficiary).revoke([this.token.target]);
  
  // At 1/2 duration: vested = 50, released = 25, unvested = 50
  const unvested = ethers.parseEther('50');
  
  await expect(tx).to.emit(this.mock, 'VestingRevoked').withArgs(this.beneficiary.address);
  await expect(tx).to.changeEtherBalances([this.mock, this.beneficiary], [-unvested, unvested]);
  await expect(tx).to.changeTokenBalances(this.token, [this.mock, this.beneficiary], [-unvested, unvested]);
  
  // Beneficiary should only be able to release the remaining vested amount (50 - 25 = 25)
  const releasableEth = await this.mock.releasable();
  expect(releasableEth).to.equal(ethers.parseEther('25'));
});

This test would verify that when some amount has been released before revocation, the owner still receives the correct unvested portion.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/finance/VestingWalletRevocable.test.js` around lines 77 - 86, Add a test
that covers revocation after a partial release to ensure unvested calculation
accounts for already released amounts: simulate time to 1/4 duration, call
release() as the beneficiary, record released amount, advance to 1/2 duration,
call revoke() and assert VestingRevoked event, that
changeEtherBalances/changeTokenBalances transfer only the unvested amount (total
vested at 1/2 minus already released), and finally assert releasable() equals
remaining vested (vested - released); use the existing test pattern and symbols
revoke(), release(), VestingRevoked, releasable() and this.mock/this.token to
implement the checks.


it('freezes vested amount at revocation timestamp', async function () {
const revokeAt = this.start + this.duration / 4n;
await time.increaseTo.timestamp(revokeAt, false);
const vestedAtRevoke = await this.mock.vestedAmount(revokeAt);
await this.mock.connect(this.beneficiary).revoke([this.token.target]);

await time.increaseTo.timestamp(this.start + this.duration);
expect(await this.mock.vestedAmount(this.start + this.duration)).to.equal(vestedAtRevoke);
});

it('keeps un-listed token vesting against on-chain balance', async function () {
const other = await ethers.deployContract('$ERC20', ['Other', 'OTH']);
await other.$_mint(this.mock, ethers.parseEther('100'));

await time.increaseTo.timestamp(this.start + this.duration / 4n, false);
await this.mock.connect(this.beneficiary).revoke([this.token.target]);

await time.increaseTo.timestamp(this.start + this.duration);
expect(await this.mock.releasable(ethers.Typed.address(other))).to.equal(ethers.parseEther('25'));
});
});
});
Loading