feat: add rVirtual conversion via RVirtualConverter - #181
Open
koo-virtuals wants to merge 16 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.
…initialize RVirtualConverter's 1:1 conversion is a raw-integer transfer with no decimals rescaling - correct only when both tokens use the same decimals. The invariant previously lived only in a comment; a future deployment against a mismatched pair (e.g. 6-decimal vs 18-decimal) would have mis-scaled every conversion silently. Now asserted via IERC20Metadata.decimals() at initialize(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… call convertVeVirtualToRVirtual() relied on the converter itself consuming its forceApprove'd allowance via transferFrom. A converter that doesn't pull the funds (whether by bug or a no-op/malicious replacement) left the full permission standing indefinitely - including after a later repoint to a different converter, since forceApprove only ever writes to the *current* converter address. veVirtual now clears the allowance itself right after the call, regardless of what the converter did with it. Updated the existing no-op-converter test (previously asserted the residual allowance as documented, expected behavior) to instead assert it's cleared, and added a dedicated test for the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
convertVeVirtualToRVirtual() deleted the caller's lock, called the converter, and returned - never confirming the caller was actually paid. That guarantee rested entirely on a check inside RVirtualConverter, a separate contract that is upgradeable behind the same ADMIN_ROLE that can also repoint rVirtualConverter (H-1). A staker forfeiting a position after a deliberate or mistaken repoint/upgrade could lose the entire locked position and receive nothing, with the transaction still reporting success. veVirtual now confirms delivery itself: - setRVirtualConverter() snapshots the converter's declared rVirtualToken() into a new `rVirtualToken` storage slot at the moment the converter is wired in (same trust boundary as the existing virtualToken() check) - not re-read from the converter at conversion time, so a later in-place upgrade of the same converter can't simply claim a different payout token to dodge the check. - convertVeVirtualToRVirtual() snapshots the account's rVirtualToken balance before the external call and requires the exact expected amount was delivered after it. A shortfall reverts the whole transaction, including the lock deletion, so the account keeps its position instead of losing it silently. This substantially reduces H-1's impact: a redirected/upgraded converter can now only cause conversions to revert (denial of service on this one feature), not fund loss - so H-1 does not additionally need a timelock to be considered adequately mitigated for now. Added `rVirtualToken()` to IRVirtualConverter (RVirtualConverter.sol already exposes it as a public state variable, satisfying the interface with no contract change needed). Updated NoOpConverterMock/MaliciousConverterMock constructors to supply it, and added NonConsumingConverterMock plus dedicated regression tests: two prove a non-delivering converter (no-op and fund-stealing) now reverts and preserves the lock, one proves normal conversions against the real converter are unaffected. The existing L-04 allowance tests were rewritten against NonConsumingConverterMock, since a non-delivering converter no longer reaches a successful call at all (the whole point of this fix) and can therefore no longer exercise the allowance-clearing path - only a converter that delivers correctly through a path that skips consuming the allowance still can. No deploy script changes needed: initialize() signatures are unchanged, and rVirtualToken is only ever set via setRVirtualConverter(), same as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
New token movement and veVirtual lock deletion depend on an admin-wired, upgradeable external converter; mitigations are on-chain delivery checks and allowance clearing, but misconfiguration or compromised admin keys remain high-impact.
Overview
Introduces a permissionless 1:1 VIRTUAL → rVirtual path via a new UUPS
RVirtualConverter: callers swap VIRTUAL (sent straight to a fixed treasury, not held in the converter) for pre-funded rVirtual, with balance-delta checks so fee-on-transfer payout tokens cannot silently under-deliver. Initialization enforces matching token decimals and distinct input/output addresses; admin sweep paths are omitted by design.veVirtualgains adminsetRVirtualConverter(base-token match + snapshottedrVirtualToken) and userconvertVeVirtualToRVirtual, which removes a lock (any maturity / auto-renew state), forwards VIRTUAL through the same open converter entrypoint, verifies full rVirtual delivery against the snapshot (reverting the whole tx—including lock removal—on shortfall), usesforceApproveand clears allowance after the call, and burns voting power._disableInitializersconstructors are added on upgradeable implementations.Hardhat coverage adds converter + integration tests and audit-focused mocks (malicious / no-op / non-consuming converters, fee-on-transfer token, UUPS round-trip).
Reviewed by Cursor Bugbot for commit d7cb747. Bugbot is set up for automated code reviews on this repo. Configure here.