Feature/vesting wallet revocable#6523
Conversation
|
WalkthroughThis PR adds revocation capability to vesting wallets via a new 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
contracts/finance/VestingWalletRevocable.sol (1)
49-51: 💤 Low valueHandle duplicate tokens in the array.
If the
tokensarray contains duplicates, the current implementation will:
- Snapshot the same token multiple times (line 50), with later iterations overwriting earlier snapshots
- Attempt to transfer unvested amounts multiple times (lines 57-61)
The second transfer will typically result in
unvested = 0because the balance was already reduced by the first transfer, so it gracefully avoids double-transfer. However, this wastes gas.Consider either:
- Documenting that duplicates are handled but waste gas
- Adding duplicate detection with a revert for better UX
📝 Example: Document the behavior
/** * `@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. + * + * Note: Duplicate addresses in `tokens` are handled gracefully but waste gas. Each duplicate will + * re-snapshot and re-check the same token. * * Emits a {VestingRevoked} event. */Also applies to: 57-61
🤖 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 49 - 51, The tokens array may contain duplicates causing redundant snapshot writes to _erc20AllocationSnapshot and wasted transfers when computing unvested; update the logic in VestingWalletRevocable (the loop that sets _erc20AllocationSnapshot[tokens[i]] = IERC20(tokens[i]).balanceOf(address(this)) + released(tokens[i]) and the subsequent unvested/release logic) to detect duplicates: maintain a local memory mapping(address => bool) seen and either revert with a clear message if a token is seen twice or skip processing duplicates by continuing when seen[tokens[i]] is true; apply the same duplicate check in both the snapshot loop and the transfer/unvested loop so duplicate tokens are never reprocessed.test/finance/VestingWalletRevocable.test.js (1)
98-107: 💤 Low valueDocumentation could clarify that unlisted tokens use frozen timestamp, not progressing time.
The test correctly validates that unlisted tokens have their vesting amount calculated using the current on-chain balance but with the timestamp frozen at revocation. The implementation confirms this: for tokens without a snapshot (unlisted),
vestedAmountcalls the parent class with_revokedAtrather than the current timestamp.The documentation at lines 38-39 uses "continue to vest against their on-chain balance at the revocation timestamp," which could be misinterpreted as ongoing vesting. Consider rewording to clarify that vesting is calculated using a frozen timestamp and current balance, not progressing time.
🤖 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 98 - 107, Update the documentation text that currently says "continue to vest against their on-chain balance at the revocation timestamp" to explicitly state that for unlisted tokens VestingWalletRevocable freezes the timestamp at _revokedAt when computing vestedAmount (via the parent class) while still using the token's current on-chain balance for releasable calculations; mention that time does not progress for vesting calculations after revoke (i.e., vesting is computed with the frozen revocation timestamp, not a moving current time), and reference the behavior in vestedAmount and releasable so readers can locate the implementation.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@contracts/finance/VestingWalletRevocable.sol`:
- Around line 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.
In `@test/finance/VestingWalletRevocable.test.js`:
- Around line 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.
---
Nitpick comments:
In `@contracts/finance/VestingWalletRevocable.sol`:
- Around line 49-51: The tokens array may contain duplicates causing redundant
snapshot writes to _erc20AllocationSnapshot and wasted transfers when computing
unvested; update the logic in VestingWalletRevocable (the loop that sets
_erc20AllocationSnapshot[tokens[i]] = IERC20(tokens[i]).balanceOf(address(this))
+ released(tokens[i]) and the subsequent unvested/release logic) to detect
duplicates: maintain a local memory mapping(address => bool) seen and either
revert with a clear message if a token is seen twice or skip processing
duplicates by continuing when seen[tokens[i]] is true; apply the same duplicate
check in both the snapshot loop and the transfer/unvested loop so duplicate
tokens are never reprocessed.
In `@test/finance/VestingWalletRevocable.test.js`:
- Around line 98-107: Update the documentation text that currently says
"continue to vest against their on-chain balance at the revocation timestamp" to
explicitly state that for unlisted tokens VestingWalletRevocable freezes the
timestamp at _revokedAt when computing vestedAmount (via the parent class) while
still using the token's current on-chain balance for releasable calculations;
mention that time does not progress for vesting calculations after revoke (i.e.,
vesting is computed with the frozen revocation timestamp, not a moving current
time), and reference the behavior in vestedAmount and releasable so readers can
locate the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0dc6742b-87c8-4434-b70a-b3474d9f1e18
📒 Files selected for processing (2)
contracts/finance/VestingWalletRevocable.soltest/finance/VestingWalletRevocable.test.js
| 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); | ||
| } |
There was a problem hiding this comment.
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 - vestedAmountHowever, 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.
| 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; | ||
| }); |
There was a problem hiding this comment.
🛠️ 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.
1 participant
Issue: #6493
Added a VestingWalletRevocable extension which provides revoking functionalities. The following constraints for revoking include: once a contract has been revoked it cannot be reinstated and revocation returns unvested tokens to owners while vested tokens remain claimable.
PR Checklist