feat: add rVirtual conversion via RVirtualConverter - #181
Open
koo-virtuals wants to merge 13 commits into
Open
Conversation
Adds an open, permissionless 1:1 VIRTUAL -> rVirtual converter (RVirtualConverter, UUPS upgradeable), pre-funded with the rVirtual supply. veVirtual gains convertVeVirtualToRVirtual(id), which deletes a staking position (regardless of maturity or autoRenew state) and routes its underlying VIRTUAL through the same open converter entrypoint any wallet can call directly - no backend distribution step required. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b98ca31. Configure here.
Compute the delivered amount via a balanceOf delta around the transfer and require it equals the requested amount, so a taxed/fee-on-transfer rVirtual token causes the whole conversion to revert atomically instead of silently under-delivering while the caller is charged in full. The emitted event also now reports the actually-delivered amount rather than the requested one. Addresses AUDIT_REPORT.md finding M-01.
No code check added by design - the protocol guarantees both VIRTUAL and rVirtual use 18 decimals, so a runtime decimals() equivalence assertion isn't needed. This comment records that assumption at the exact line the raw 1:1 conversion depends on it, so a future migration to a differently-decimaled token isn't wired in silently. Addresses AUDIT_REPORT.md finding M-02 (comment-only fix per request; no behavior change, no new test).
Add a `treasury` address, required non-zero and set once at initialize(), and change convertVirtualToRVirtual() to send the caller's incoming VIRTUAL straight to it instead of holding it in the converter contract. VIRTUAL is no longer custodied by RVirtualConverter at any point, so there is no accumulated balance for a compromised/malicious ADMIN_ROLE key to sweep via a repointed converter or a malicious UUPS upgrade. This does not change the original L-02 attack surface (a malicious `rVirtualConverter` repoint on veVirtual, or a malicious UUPS upgrade, can still intercept a user's approved VIRTUAL before it reaches treasury) - that remains an acknowledged, trust-gated risk. It does remove the "accumulated VIRTUAL balance" component of the blast radius entirely, since funds never sit in this contract. Addresses AUDIT_REPORT.md finding L-02 (additional hardening beyond the original recommendation, per request).
convertVeVirtualToRVirtual() now grants the converter's allowance via forceApprove(), consistent with the rest of the file's SafeERC20 convention (already imported/used elsewhere via `using SafeERC20 for IERC20`), and checked/return-value-safe by construction. forceApprove zeroes the allowance first if it's currently non-zero, then sets the new value - this is a strict superset of what raw approve() does for well-behaved tokens like VIRTUAL, so there is no behavior change on the happy path; it additionally protects against ERC20 tokens that revert on a direct non-zero-to-non-zero approve() (e.g. USDT-style tokens), should baseToken ever be swapped for one of those. Addresses AUDIT_REPORT.md finding L-04.
RVirtualConverter.initialize() now rejects virtualToken and rVirtualToken being the same address. IRVirtualConverter exposes a virtualToken() getter, and veVirtual.setRVirtualConverter() now asserts the converter's virtualToken() matches veVirtual's own baseToken before wiring it in - closing the gap where a misconfigured/mismatched converter would previously only fail with a confusing revert deep inside a user's convertVeVirtualToRVirtual() call instead of at configuration time. NoOpConverterMock gains a constructor-supplied virtualToken to satisfy the extended interface. Addresses AUDIT_REPORT.md finding L-09.
FeeOnTransferMock, MaliciousConverterMock, and MockERC20SixDecimals were created during the earlier security audit's PoC work but never added to git. FeeOnTransferMock is a dependency of the M-01 fix test added in an earlier commit; tracking all three now so the test suite is reproducible from a clean checkout.
RVirtualConverter's AdminWalletUpdated event now emits both the previous and new adminWallet, both indexed, so an off-chain monitor can filter directly on this state change and reconstruct history without replaying the full event log. veVirtual's own admin events (RVirtualConverterUpdated, and the absence of an event on setMaxWeeks) are intentionally left unchanged per request. Addresses AUDIT_REPORT.md finding L-10 (RVirtualConverter side only).
…cks/ Test-only UUPS upgrade-path mock was sitting alongside production contracts in contracts/token/. Relocated with the other test-only mocks for this feature; no behavior change. Addresses AUDIT_REPORT.md finding I-05.
Once incoming VIRTUAL is routed directly to treasury (L-02), this contract never custodies VIRTUAL, so the adminWallet sweep mechanism (setAdminWallet, withdrawVirtual, and their events) has nothing left to do. Removed entirely rather than left as dead code with no consumer - narrows the admin-controlled surface down to just the UUPS upgrade path. Addresses AUDIT_REPORT.md finding I-06.
Add constructor() { _disableInitializers(); } to both
RVirtualConverter and veVirtual, matching the standard OZ upgradeable
hardening pattern already used by every other upgradeable contract in
this repo (AgentVeToken, Bonding, and 35+ others). Without this, the
implementation contract itself (as opposed to any proxy pointing to
it) could be initialized directly by an attacker, granting them
DEFAULT_ADMIN_ROLE/ADMIN_ROLE over the naked implementation.
Addresses AUDIT_REPORT.md finding L-12.
…tate
Simulates the real-world scenario: an already-live veVirtual proxy
with existing stakes/votes gets upgraded to a new implementation that
adds constructor() { _disableInitializers(); }. Forces a genuinely
new implementation deployment (redeployImplementation: "always") and
asserts every pre-upgrade storage value (lock data, decayed balance,
raw voting power) survives untouched, and that the new implementation
is independently hardened against direct initialize() calls.
This is empirical proof (not just theory) that adding this
constructor to a contract that has already been deployed and upgraded
multiple times on mainnet is safe: constructors only ever run once,
at the new implementation's own deployment transaction, and never
execute in the context of - or touch the storage of - the proxy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an open, permissionless 1:1 VIRTUAL -> rVirtual converter (RVirtualConverter, UUPS upgradeable), pre-funded with the rVirtual supply. veVirtual gains convertVeVirtualToRVirtual(id), which deletes a staking position (regardless of maturity or autoRenew state) and routes its underlying VIRTUAL through the same open converter entrypoint any wallet can call directly - no backend distribution step required.
Note
High Risk
Changes core token flows and veVirtual staking/voting: locks are deleted before the external converter call, so a malicious or misconfigured converter could strand users unless admin wiring and liquidity are correct.
Overview
Introduces a permissionless 1:1 VIRTUAL → rVirtual path via a new UUPS
RVirtualConverter: callers pull VIRTUAL to a fixed treasury, and the contract pays pre-funded rVirtual with a balance-delta check so fee-on-transfer rVirtual cannot silently under-deliver. Admin sweep paths are intentionally omitted because VIRTUAL is not held on the converter.veVirtualgainsconvertVeVirtualToRVirtual(id), which removes a lock (including immature and auto-renew positions), burns voting power,forceApproves the configured converter, and calls the same openconvertVirtualToRVirtualentrypoint wallets use. Admins wire the converter withsetRVirtualConverter, which requiresvirtualToken()to matchbaseToken. BothveVirtualandRVirtualConverteradd_disableInitializerson the implementation constructor.Adds Hardhat coverage for converter behavior, treasury routing, UUPS upgrades, and veVirtual conversion edge cases, plus test-only converter/token mocks for audit scenarios.
.gitignorenow ignoreslib/anddocs/.Reviewed by Cursor Bugbot for commit 5a85940. Bugbot is set up for automated code reviews on this repo. Configure here.