diff --git a/.gitignore b/.gitignore index eb15dcff..242d89d0 100644 --- a/.gitignore +++ b/.gitignore @@ -22,4 +22,5 @@ fireblocks_secret.key .cursor/ env* -lib/ \ No newline at end of file +lib/ +docs/ \ No newline at end of file diff --git a/contracts/token/IRVirtualConverter.sol b/contracts/token/IRVirtualConverter.sol new file mode 100644 index 00000000..6592c1c6 --- /dev/null +++ b/contracts/token/IRVirtualConverter.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +interface IRVirtualConverter { + function convertVirtualToRVirtual( + uint256 amount, + address rVirtualReceiver + ) external; + + /// @notice The VIRTUAL token this converter accepts as input. Exposed so callers + /// (e.g. veVirtual.setRVirtualConverter) can assert their own base token + /// matches this converter's before wiring it in (see audit L-09). + function virtualToken() external view returns (address); +} diff --git a/contracts/token/RVirtualConverter.sol b/contracts/token/RVirtualConverter.sol new file mode 100644 index 00000000..17d227c2 --- /dev/null +++ b/contracts/token/RVirtualConverter.sol @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; +import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; +import "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol"; +import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; +import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; + +/// @notice Open, permissionless 1:1 converter from VIRTUAL to rVirtual. +/// +/// Pre-funded with the full rVirtual supply before launch - conversions draw down that +/// balance rather than minting on demand. Any caller (a regular wallet, or veVirtual's +/// convertVeVirtualToRVirtual()) uses the exact same convertVirtualToRVirtual() entrypoint; +/// there is no privileged "veVirtual-only" path here. +contract RVirtualConverter is + Initializable, + ReentrancyGuardUpgradeable, + AccessControlUpgradeable, + UUPSUpgradeable +{ + using SafeERC20 for IERC20; + + bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE"); + + address public virtualToken; + address public rVirtualToken; + /// @notice Treasury multisig that every conversion's incoming VIRTUAL is sent to + /// directly. Set once at initialize() and never changed at runtime - VIRTUAL + /// is never custodied by this contract, so there is no accumulated balance + /// for a compromised or malicious admin key to sweep (see audit L-02). This + /// is also why there is no adminWallet/withdrawVirtual() sweep mechanism here + /// anymore (see audit I-06) - there is nothing left for it to sweep. + address public treasury; + + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + + event ConvertedVirtualToRVirtual( + address indexed caller, + address indexed rVirtualReceiver, + uint256 amount + ); + + function initialize( + address virtualToken_, + address rVirtualToken_, + address treasury_ + ) external initializer { + __ReentrancyGuard_init(); + __AccessControl_init(); + __UUPSUpgradeable_init(); + + require(virtualToken_ != address(0), "Invalid virtual token"); + require(rVirtualToken_ != address(0), "Invalid rVirtual token"); + require(treasury_ != address(0), "Invalid treasury"); + require(virtualToken_ != rVirtualToken_, "Tokens must differ"); + // NOTE (audit M-02): the 1:1 conversion below is a raw-integer transfer with no + // decimals rescaling. This is safe only because VIRTUAL and rVirtual are both + // guaranteed by protocol design to use 18 decimals - if either token is ever + // redeployed/migrated to a different decimals value, this invariant must be + // re-verified (or an explicit decimals() equivalence check added) before wiring + // it in here. + virtualToken = virtualToken_; + rVirtualToken = rVirtualToken_; + treasury = treasury_; + + _grantRole(DEFAULT_ADMIN_ROLE, _msgSender()); + _grantRole(ADMIN_ROLE, _msgSender()); + } + + /// @notice Convert `amount` VIRTUAL (pulled from the caller) into `amount` rVirtual, + /// sent to `rVirtualReceiver`. Fully open - no allowlist, no cap. + function convertVirtualToRVirtual( + uint256 amount, + address rVirtualReceiver + ) external nonReentrant { + require(amount > 0, "Amount must be greater than 0"); + require(rVirtualReceiver != address(0), "Invalid receiver"); + + IERC20(virtualToken).safeTransferFrom( + _msgSender(), + treasury, + amount + ); + + uint256 balanceBefore = IERC20(rVirtualToken).balanceOf(rVirtualReceiver); + IERC20(rVirtualToken).safeTransfer(rVirtualReceiver, amount); + uint256 delivered = IERC20(rVirtualToken).balanceOf(rVirtualReceiver) - balanceBefore; + require(delivered == amount, "rVirtual delivery mismatch"); + + emit ConvertedVirtualToRVirtual(_msgSender(), rVirtualReceiver, delivered); + } + + function _authorizeUpgrade( + address newImplementation + ) internal override onlyRole(ADMIN_ROLE) {} +} diff --git a/contracts/token/mocks/FeeOnTransferMock.sol b/contracts/token/mocks/FeeOnTransferMock.sol new file mode 100644 index 00000000..ab2787c6 --- /dev/null +++ b/contracts/token/mocks/FeeOnTransferMock.sol @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; + +/// @notice TEST-ONLY mock modeling a fee-on-transfer / tax token, used to simulate the +/// real rVirtual (GodToken)'s configurable transfer tax for security PoC purposes +/// (see verify_H-2.js). NOT used in production - lives under contracts/token/mocks +/// solely to be reachable by Hardhat's compiler for the test. +/// +/// Deducts `feeBps` (out of 10_000) from every transfer/transferFrom, sending the +/// fee portion to a burn/dead sink so the recipient always receives strictly less +/// than the nominal transferred amount whenever feeBps > 0. +contract FeeOnTransferMock is ERC20 { + uint256 public immutable feeBps; // e.g. 1000 = 10% + address public immutable feeSink; + + constructor( + string memory name_, + string memory symbol_, + address initialAccount, + uint256 initialBalance, + uint256 feeBps_, + address feeSink_ + ) ERC20(name_, symbol_) { + require(feeBps_ <= 10_000, "fee too high"); + feeBps = feeBps_; + feeSink = feeSink_; + _mint(initialAccount, initialBalance); + } + + function _update(address from, address to, uint256 value) internal override { + // Mint (from == address(0)) and burn (to == address(0)) pass through untaxed - + // only regular transfers between two live accounts are taxed, matching typical + // fee-on-transfer token behavior. + if (from == address(0) || to == address(0) || feeBps == 0) { + super._update(from, to, value); + return; + } + + uint256 fee = (value * feeBps) / 10_000; + uint256 net = value - fee; + + super._update(from, to, net); + if (fee > 0) { + super._update(from, feeSink, fee); + } + } +} diff --git a/contracts/token/mocks/MaliciousConverterMock.sol b/contracts/token/mocks/MaliciousConverterMock.sol new file mode 100644 index 00000000..084e3879 --- /dev/null +++ b/contracts/token/mocks/MaliciousConverterMock.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "../IRVirtualConverter.sol"; + +/// @notice TEST-ONLY mock simulating a malicious/repointed `rVirtualConverter` (see H-4 PoC, +/// verify_H-4.js). Implements the exact `IRVirtualConverter` interface that +/// `veVirtual.convertVeVirtualToRVirtual()` calls, but instead of delivering rVirtual +/// 1:1, it pulls the approved VIRTUAL via `transferFrom` and routes it to an +/// attacker-controlled address, delivering ZERO rVirtual back to the victim. It does +/// NOT revert - the call succeeds silently from veVirtual's perspective, so the +/// lock deletion (which already happened before this external call) is never rolled +/// back. +contract MaliciousConverterMock is IRVirtualConverter { + address public immutable virtualToken; + address public immutable attacker; + + constructor(address virtualToken_, address attacker_) { + virtualToken = virtualToken_; + attacker = attacker_; + } + + function convertVirtualToRVirtual( + uint256 amount, + address /* rVirtualReceiver */ + ) external override { + // Steal the approved VIRTUAL; deliver no rVirtual, and do not revert. + IERC20(virtualToken).transferFrom(msg.sender, attacker, amount); + } +} diff --git a/contracts/token/mocks/MockERC20SixDecimals.sol b/contracts/token/mocks/MockERC20SixDecimals.sol new file mode 100644 index 00000000..b187358c --- /dev/null +++ b/contracts/token/mocks/MockERC20SixDecimals.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; + +/// @notice Minimal mock ERC20 with 6 decimals (like USDC), used ONLY to verify H-3 +/// (RVirtualConverter never asserts rVirtualToken.decimals() == 18). +contract MockERC20SixDecimals is ERC20 { + constructor( + string memory name, + string memory symbol, + address initialAccount, + uint256 initialBalance + ) ERC20(name, symbol) { + _mint(initialAccount, initialBalance); + } + + function decimals() public pure override returns (uint8) { + return 6; + } + + function mint(address to, uint256 amount) public { + _mint(to, amount); + } +} diff --git a/contracts/token/mocks/NoOpConverterMock.sol b/contracts/token/mocks/NoOpConverterMock.sol new file mode 100644 index 00000000..e3690154 --- /dev/null +++ b/contracts/token/mocks/NoOpConverterMock.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import "../IRVirtualConverter.sol"; + +/// @notice TEST-ONLY mock for verify_H-10.js. Implements IRVirtualConverter but does NOT call +/// transferFrom at all - simulates the specific scenario Chain Agent 2 analyzed where +/// the converter call succeeds (no-op) without pulling the approved VIRTUAL, leaving +/// veVirtual's raw `approve()` allowance (L345) standing after the call returns. +/// Distinct from MaliciousConverterMock (H-4), which DOES pull funds via transferFrom +/// and therefore leaves zero residual allowance. +contract NoOpConverterMock is IRVirtualConverter { + address public virtualToken; + + constructor(address virtualToken_) { + virtualToken = virtualToken_; + } + + function convertVirtualToRVirtual( + uint256 /* amount */, + address /* rVirtualReceiver */ + ) external override { + // Intentionally does nothing - no transferFrom, no revert. + } +} diff --git a/contracts/token/mocks/RVirtualConverterV2Mock.sol b/contracts/token/mocks/RVirtualConverterV2Mock.sol new file mode 100644 index 00000000..040d7e0d --- /dev/null +++ b/contracts/token/mocks/RVirtualConverterV2Mock.sol @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import "../RVirtualConverter.sol"; + +/// @notice Test-only V2 used to verify RVirtualConverter's UUPS upgrade path round-trips +/// cleanly. Adds one new event + trigger function on top of V1; never deployed to +/// production - exists purely so a test can upgrade forward, prove the new code is +/// live, then upgrade back and confirm the final bytecode matches the original V1. +contract RVirtualConverterV2Mock is RVirtualConverter { + event V2UpgradeMarker(string message); + + function triggerV2Marker() external { + emit V2UpgradeMarker("upgraded"); + } +} diff --git a/contracts/token/veVirtual.sol b/contracts/token/veVirtual.sol index 1de0c488..1fae635d 100644 --- a/contracts/token/veVirtual.sol +++ b/contracts/token/veVirtual.sol @@ -7,6 +7,7 @@ import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import "@openzeppelin/contracts/utils/structs/Checkpoints.sol"; import "@openzeppelin/contracts-upgradeable/governance/utils/VotesUpgradeable.sol"; +import "./IRVirtualConverter.sol"; contract veVirtual is Initializable, @@ -34,6 +35,11 @@ contract veVirtual is uint8 public maxWeeks; + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + event Stake( address indexed user, uint256 id, @@ -47,6 +53,14 @@ contract veVirtual is event AdminUnlocked(bool adminUnlocked); bool public adminUnlocked; + address public rVirtualConverter; + event RVirtualConverterUpdated(address rVirtualConverter); + event ConvertedVeVirtualToRVirtual( + address indexed user, + uint256 id, + uint256 amount + ); + function initialize( address baseToken_, uint8 maxWeeks_ @@ -298,4 +312,52 @@ contract veVirtual is } return amount; } + + /** + * @notice Set the RVirtualConverter contract that convertVeVirtualToRVirtual() forwards to. + */ + function setRVirtualConverter( + address rVirtualConverter_ + ) external onlyRole(ADMIN_ROLE) { + require(rVirtualConverter_ != address(0), "Invalid converter"); + require( + IRVirtualConverter(rVirtualConverter_).virtualToken() == baseToken, + "Converter token mismatch" + ); + rVirtualConverter = rVirtualConverter_; + emit RVirtualConverterUpdated(rVirtualConverter_); + } + + /** + * @notice Voluntarily give up a lock's underlying VIRTUAL (and its voting power) in + * exchange for an equal amount of rVirtual. Unlike withdraw(), this does not + * require the lock to be matured - the user is explicitly forfeiting the + * remaining lock time. The lock is deleted regardless of its autoRenew state. + * @dev Approves RVirtualConverter for exactly this lock's amount and calls its + * convertVirtualToRVirtual() - the same open entrypoint any wallet can call directly. + * There is no veVirtual-specific path on the converter side. + */ + function convertVeVirtualToRVirtual(uint256 id) external nonReentrant { + require(rVirtualConverter != address(0), "Converter not set"); + address account = _msgSender(); + uint256 index = _indexOf(account, id); + Lock memory lock = locks[account][index]; + + uint256 amount = lock.amount; + + uint256 lastIndex = locks[account].length - 1; + if (index != lastIndex) { + locks[account][index] = locks[account][lastIndex]; + } + locks[account].pop(); + + IERC20(baseToken).forceApprove(rVirtualConverter, amount); + IRVirtualConverter(rVirtualConverter).convertVirtualToRVirtual( + amount, + account + ); + + emit ConvertedVeVirtualToRVirtual(account, id, amount); + _transferVotingUnits(account, address(0), amount); + } } diff --git a/test/rvirtual-converter.js b/test/rvirtual-converter.js new file mode 100644 index 00000000..af829253 --- /dev/null +++ b/test/rvirtual-converter.js @@ -0,0 +1,285 @@ +/* +Test RVirtualConverter: an open, permissionless 1:1 VIRTUAL -> rVirtual converter. +Also verifies the UUPS upgrade path round-trips cleanly (upgrade forward, verify new code +is live, upgrade back, confirm final bytecode matches the original). +*/ +const { expect } = require("chai"); +const { ethers, upgrades } = require("hardhat"); +const { parseEther } = ethers; + +describe("RVirtualConverter", function () { + let virtual, rVirtual, converter; + let deployer, user, other, treasury; + + before(async function () { + [deployer, user, other, treasury] = await ethers.getSigners(); + }); + + beforeEach(async function () { + virtual = await ethers.deployContract("VirtualToken", [ + parseEther("1000000000"), + deployer.address, + ]); + rVirtual = await ethers.deployContract("MockERC20", [ + "rVirtual", + "rVIRTUAL", + deployer.address, + parseEther("1000000000"), + ]); + + const Converter = await ethers.getContractFactory("RVirtualConverter"); + converter = await upgrades.deployProxy(Converter, [ + virtual.target, + rVirtual.target, + treasury.address, + ]); + + // Pre-fund the converter with rVirtual liquidity for these tests (production pre-funds + // the full 1B supply, but a smaller amount here leaves deployer enough spare VIRTUAL + // balance to fully drain it in the "insufficient liquidity" test below). + await rVirtual.transfer(converter.target, parseEther("10000")); + + await virtual.transfer(user.address, parseEther("1000")); + await virtual.connect(user).approve(converter.target, parseEther("1000")); + }); + + describe("convertVirtualToRVirtual", function () { + it("should convert at a flat 1:1 rate for any caller", async function () { + await expect( + converter.connect(user).convertVirtualToRVirtual(parseEther("100"), user.address) + ) + .to.emit(converter, "ConvertedVirtualToRVirtual") + .withArgs(user.address, user.address, parseEther("100")); + + expect(await virtual.balanceOf(user.address)).to.be.equal(parseEther("900")); + // Incoming VIRTUAL is routed straight to the treasury (L-02 fix) - the converter + // itself never custodies any VIRTUAL. + expect(await virtual.balanceOf(converter.target)).to.be.equal(0); + expect(await virtual.balanceOf(treasury.address)).to.be.equal(parseEther("100")); + expect(await rVirtual.balanceOf(user.address)).to.be.equal(parseEther("100")); + }); + + it("should allow sending the rVirtual to a different receiver than the caller", async function () { + await converter + .connect(user) + .convertVirtualToRVirtual(parseEther("100"), other.address); + + expect(await rVirtual.balanceOf(other.address)).to.be.equal(parseEther("100")); + expect(await rVirtual.balanceOf(user.address)).to.be.equal(0); + }); + + it("should be fully open - no allowlist, no cap", async function () { + // Large relative to a normal user conversion, but within the test pool's pre-funded + // rVirtual liquidity (10000) - the point here is the absence of any allowlist/cap + // check, not exercising the liquidity limit (covered separately below). + await virtual.transfer(other.address, parseEther("5000")); + await virtual.connect(other).approve(converter.target, parseEther("5000")); + + await expect( + converter.connect(other).convertVirtualToRVirtual(parseEther("5000"), other.address) + ).to.not.be.reverted; + expect(await rVirtual.balanceOf(other.address)).to.be.equal(parseEther("5000")); + }); + + it("should revert without prior VIRTUAL approval", async function () { + await virtual.connect(user).approve(converter.target, 0); + await expect( + converter.connect(user).convertVirtualToRVirtual(parseEther("100"), user.address) + ).to.be.reverted; + }); + + it("should revert if the converter has insufficient rVirtual liquidity", async function () { + // Drain the pre-funded rVirtual balance first. + const drained = await rVirtual.balanceOf(converter.target); + // Impersonate is unnecessary - just have deployer pull it out via a huge legit conversion + // from a funded wallet to exhaust the pool, then attempt one more. + await virtual.transfer(other.address, drained); + await virtual.connect(other).approve(converter.target, drained); + await converter.connect(other).convertVirtualToRVirtual(drained, other.address); + + expect(await rVirtual.balanceOf(converter.target)).to.be.equal(0); + + await expect( + converter.connect(user).convertVirtualToRVirtual(parseEther("100"), user.address) + ).to.be.reverted; + }); + + it("should reject a zero receiver address", async function () { + await expect( + converter.connect(user).convertVirtualToRVirtual(parseEther("100"), ethers.ZeroAddress) + ).to.be.revertedWith("Invalid receiver"); + }); + + it("should reject a zero amount", async function () { + await expect( + converter.connect(user).convertVirtualToRVirtual(0, user.address) + ).to.be.revertedWith("Amount must be greater than 0"); + }); + }); + + describe("delivery verification (M-01 fix)", function () { + let taxedRVirtual, taxedConverter; + + beforeEach(async function () { + // 10% fee-on-transfer token standing in for a taxed rVirtual configuration. + taxedRVirtual = await ethers.deployContract("FeeOnTransferMock", [ + "rVirtual", + "rVIRTUAL", + deployer.address, + parseEther("1000000000"), + 1000, // 10% fee + other.address, // fee sink + ]); + + const Converter = await ethers.getContractFactory("RVirtualConverter"); + taxedConverter = await upgrades.deployProxy(Converter, [ + virtual.target, + taxedRVirtual.target, + treasury.address, + ]); + await taxedRVirtual.transfer(taxedConverter.target, parseEther("10000")); + + await virtual.connect(user).approve(taxedConverter.target, parseEther("1000")); + }); + + it("should revert the whole conversion instead of silently under-delivering", async function () { + await expect( + taxedConverter.connect(user).convertVirtualToRVirtual(parseEther("100"), user.address) + ).to.be.revertedWith("rVirtual delivery mismatch"); + + // Both legs must have unwound - caller keeps their VIRTUAL, receives no rVirtual. + expect(await virtual.balanceOf(user.address)).to.be.equal(parseEther("1000")); + expect(await taxedRVirtual.balanceOf(user.address)).to.be.equal(0); + }); + + it("should still succeed and emit the exact delivered amount for a non-taxed token", async function () { + await expect( + converter.connect(user).convertVirtualToRVirtual(parseEther("100"), user.address) + ) + .to.emit(converter, "ConvertedVirtualToRVirtual") + .withArgs(user.address, user.address, parseEther("100")); + + expect(await rVirtual.balanceOf(user.address)).to.be.equal(parseEther("100")); + }); + }); + + describe("treasury routing (L-02 fix)", function () { + it("should reject a zero-address treasury at initialize", async function () { + const Converter = await ethers.getContractFactory("RVirtualConverter"); + await expect( + upgrades.deployProxy(Converter, [ + virtual.target, + rVirtual.target, + ethers.ZeroAddress, + ]) + ).to.be.revertedWith("Invalid treasury"); + }); + + it("should reject virtualToken and rVirtualToken being the same address (L-09 fix)", async function () { + const Converter = await ethers.getContractFactory("RVirtualConverter"); + await expect( + upgrades.deployProxy(Converter, [ + virtual.target, + virtual.target, + treasury.address, + ]) + ).to.be.revertedWith("Tokens must differ"); + }); + + it("should route every conversion's incoming VIRTUAL straight to treasury, never the converter", async function () { + await converter.connect(user).convertVirtualToRVirtual(parseEther("30"), user.address); + await converter.connect(user).convertVirtualToRVirtual(parseEther("20"), other.address); + + expect(await virtual.balanceOf(treasury.address)).to.be.equal(parseEther("50")); + expect(await virtual.balanceOf(converter.target)).to.be.equal(0); + }); + }); + + it("should provide no VIRTUAL/rVirtual sweep mechanism at all (I-06 fix)", async function () { + // withdrawVirtual()/adminWallet/setAdminWallet were removed entirely once L-02's + // treasury routing made them unnecessary - VIRTUAL is never custodied by this + // contract, so there is nothing left for an admin-controlled sweep to reach. + expect(converter.withdrawVirtual).to.be.undefined; + expect(converter.setAdminWallet).to.be.undefined; + expect(converter.adminWallet).to.be.undefined; + expect(converter.withdrawRVirtual).to.be.undefined; + expect(converter.rescueToken).to.be.undefined; + expect(converter.recoverToken).to.be.undefined; + }); + + describe("_disableInitializers on implementation (L-12 fix)", function () { + it("should reject calling initialize() directly on the raw implementation contract", async function () { + const implAddress = await upgrades.erc1967.getImplementationAddress(converter.target); + const implementation = await ethers.getContractAt("RVirtualConverter", implAddress); + + await expect( + implementation.initialize(virtual.target, rVirtual.target, treasury.address) + ).to.be.reverted; + }); + }); + + describe("UUPS upgradeability", function () { + it("should upgrade to V2, run new code, upgrade back to V1, and end up with identical bytecode", async function () { + const implBefore = await upgrades.erc1967.getImplementationAddress(converter.target); + const codeBefore = await ethers.provider.getCode(implBefore); + expect(codeBefore).to.not.equal("0x"); + + // Upgrade forward to V2. RVirtualConverterV2Mock adds no new storage or initializer - + // it only appends a function/event - so the plugin's "missing initializer" heuristic + // (which fires for any child contract without its own initialize()) is a false + // positive here and safe to bypass. + const V2 = await ethers.getContractFactory("RVirtualConverterV2Mock"); + const upgraded = await upgrades.upgradeProxy(converter.target, V2, { + unsafeAllow: ["missing-initializer"], + }); + + const implAfterV2 = await upgrades.erc1967.getImplementationAddress(upgraded.target); + expect(implAfterV2).to.not.equal(implBefore); + + // Prove the new code is actually live. + await expect(upgraded.triggerV2Marker()) + .to.emit(upgraded, "V2UpgradeMarker") + .withArgs("upgraded"); + + // Existing state and functionality must survive the upgrade untouched. + expect(await upgraded.virtualToken()).to.be.equal(virtual.target); + expect(await upgraded.rVirtualToken()).to.be.equal(rVirtual.target); + await expect( + upgraded.connect(user).convertVirtualToRVirtual(parseEther("50"), user.address) + ).to.emit(upgraded, "ConvertedVirtualToRVirtual"); + + // Upgrade back to V1. + const V1 = await ethers.getContractFactory("RVirtualConverter"); + const backToV1 = await upgrades.upgradeProxy(upgraded.target, V1); + + const implAfterRoundTrip = await upgrades.erc1967.getImplementationAddress( + backToV1.target + ); + const codeAfterRoundTrip = await ethers.provider.getCode(implAfterRoundTrip); + + // Final deployed code must match the original V1 bytecode exactly - the round trip + // (V1 -> V2 -> V1) leaves the contract functionally and byte-for-byte identical to + // where it started, even though the implementation address itself differs (each + // upgrade deploys a fresh implementation contract). + expect(codeAfterRoundTrip).to.be.equal(codeBefore); + + // V2-only function must no longer be part of the (V1) interface/ABI. + expect(backToV1.triggerV2Marker).to.be.undefined; + + // Original functionality still works post-round-trip. + expect(await backToV1.virtualToken()).to.be.equal(virtual.target); + await expect( + backToV1.connect(user).convertVirtualToRVirtual(parseEther("50"), user.address) + ).to.emit(backToV1, "ConvertedVirtualToRVirtual"); + }); + + it("should reject upgrade attempts from non-admin accounts", async function () { + const V2 = await ethers.getContractFactory("RVirtualConverterV2Mock", user); + await expect( + upgrades.upgradeProxy(converter.target, V2, { + unsafeAllow: ["missing-initializer"], + }) + ).to.be.reverted; + }); + }); +}); diff --git a/test/vevirtual-rvirtual-conversion.js b/test/vevirtual-rvirtual-conversion.js new file mode 100644 index 00000000..3d50d9a2 --- /dev/null +++ b/test/vevirtual-rvirtual-conversion.js @@ -0,0 +1,228 @@ +/* +Test veVirtual.convertVeVirtualToRVirtual(): deletes a staking position and routes its +underlying VIRTUAL through RVirtualConverter for a flat 1:1 rVirtual payout. No maturity +requirement, no autoRenew restriction - any position can be converted directly. +*/ +const { expect } = require("chai"); +const { ethers } = require("hardhat"); +const { parseEther } = ethers; +const { time } = require("@nomicfoundation/hardhat-network-helpers"); + +describe("veVIRTUAL - convertVeVirtualToRVirtual", function () { + let virtual, rVirtual, veVirtual, converter; + let deployer, staker, staker2, other, treasury; + + before(async function () { + [deployer, staker, staker2, other, treasury] = await ethers.getSigners(); + }); + + beforeEach(async function () { + virtual = await ethers.deployContract("VirtualToken", [ + parseEther("1000000000"), + deployer.address, + ]); + rVirtual = await ethers.deployContract("MockERC20", [ + "rVirtual", + "rVIRTUAL", + deployer.address, + parseEther("1000000000"), + ]); + + const VeVirtualContract = await ethers.getContractFactory("veVirtual"); + veVirtual = await upgrades.deployProxy(VeVirtualContract, [virtual.target, 104]); + + const ConverterContract = await ethers.getContractFactory("RVirtualConverter"); + converter = await upgrades.deployProxy(ConverterContract, [ + virtual.target, + rVirtual.target, + treasury.address, + ]); + await rVirtual.transfer(converter.target, parseEther("1000000000")); + + await virtual.transfer(staker.address, parseEther("1000")); + await virtual.connect(staker).approve(veVirtual.target, parseEther("1000")); + }); + + it("should reject conversion when rVirtualConverter is not set", async function () { + await veVirtual.connect(staker).stake(parseEther("100"), 52, false); + const id = (await veVirtual.locks(staker.address, 0)).id; + + await expect( + veVirtual.connect(staker).convertVeVirtualToRVirtual(id) + ).to.be.revertedWith("Converter not set"); + }); + + it("should reject non-admin setting rVirtualConverter", async function () { + await expect( + veVirtual.connect(staker).setRVirtualConverter(converter.target) + ).to.be.reverted; + }); + + it("should reject a converter whose virtualToken doesn't match veVirtual's baseToken (L-09 fix)", async function () { + // A converter wired up with a DIFFERENT token as its virtualToken (e.g. rVirtual + // itself, standing in for any mismatched deployment). + const ConverterContract = await ethers.getContractFactory("RVirtualConverter"); + const mismatchedConverter = await upgrades.deployProxy(ConverterContract, [ + rVirtual.target, + virtual.target, + other.address, + ]); + + await expect( + veVirtual.setRVirtualConverter(mismatchedConverter.target) + ).to.be.revertedWith("Converter token mismatch"); + }); + + describe("with rVirtualConverter configured", function () { + beforeEach(async function () { + await veVirtual.setRVirtualConverter(converter.target); + }); + + it("should convert a NOT-yet-matured lock at a flat 1:1 rate", async function () { + await veVirtual.connect(staker).stake(parseEther("100"), 52, false); + const id = (await veVirtual.locks(staker.address, 0)).id; + + // Confirm it's genuinely not matured - withdraw() would revert here. + await expect(veVirtual.connect(staker).withdraw(id)).to.be.revertedWith( + "Lock is not expired" + ); + + await expect(veVirtual.connect(staker).convertVeVirtualToRVirtual(id)) + .to.emit(veVirtual, "ConvertedVeVirtualToRVirtual") + .withArgs(staker.address, id, parseEther("100")); + + expect(await veVirtual.numPositions(staker.address)).to.be.equal(0); + expect(await veVirtual.balanceOf(staker.address)).to.be.equal(0); + expect(await rVirtual.balanceOf(staker.address)).to.be.equal(parseEther("100")); + // Incoming VIRTUAL is routed straight to treasury (L-02 fix), never held by the converter. + expect(await virtual.balanceOf(converter.target)).to.be.equal(0); + expect(await virtual.balanceOf(treasury.address)).to.be.equal(parseEther("100")); + }); + + it("should also convert an already-matured lock", async function () { + await veVirtual.connect(staker).stake(parseEther("100"), 52, false); + const id = (await veVirtual.locks(staker.address, 0)).id; + await time.increase(53 * 7 * 24 * 60 * 60); + + await veVirtual.connect(staker).convertVeVirtualToRVirtual(id); + expect(await rVirtual.balanceOf(staker.address)).to.be.equal(parseEther("100")); + }); + + it("should convert an auto-renewing lock too - no autoRenew restriction", async function () { + await veVirtual.connect(staker).stake(parseEther("100"), 52, true); + const id = (await veVirtual.locks(staker.address, 0)).id; + + await expect(veVirtual.connect(staker).convertVeVirtualToRVirtual(id)).to.not.be + .reverted; + expect(await rVirtual.balanceOf(staker.address)).to.be.equal(parseEther("100")); + expect(await veVirtual.numPositions(staker.address)).to.be.equal(0); + }); + + it("should convert regardless of remaining lock time, always at 1:1", async function () { + await virtual.transfer(staker.address, parseEther("100")); + await veVirtual.connect(staker).stake(parseEther("100"), 104, false); + const id = (await veVirtual.locks(staker.address, 0)).id; + + // Fresh position, maximum remaining lock time - still full 1:1, no discount. + await veVirtual.connect(staker).convertVeVirtualToRVirtual(id); + expect(await rVirtual.balanceOf(staker.address)).to.be.equal(parseEther("100")); + }); + + it("should not allow converting someone else's lock id", async function () { + await veVirtual.connect(staker).stake(parseEther("100"), 52, false); + const id = (await veVirtual.locks(staker.address, 0)).id; + + await expect( + veVirtual.connect(staker2).convertVeVirtualToRVirtual(id) + ).to.be.revertedWith("Lock not found"); + }); + + it("should not allow withdrawing or re-converting after conversion", async function () { + await veVirtual.connect(staker).stake(parseEther("100"), 52, false); + const id = (await veVirtual.locks(staker.address, 0)).id; + await veVirtual.connect(staker).convertVeVirtualToRVirtual(id); + + await expect(veVirtual.connect(staker).withdraw(id)).to.be.revertedWith( + "Lock not found" + ); + await expect( + veVirtual.connect(staker).convertVeVirtualToRVirtual(id) + ).to.be.revertedWith("Lock not found"); + }); + + it("should only remove the converted lock, leaving other positions untouched", async function () { + await veVirtual.connect(staker).stake(parseEther("100"), 52, false); // id 1 + await veVirtual.connect(staker).stake(parseEther("50"), 52, false); // id 2 + expect(await veVirtual.numPositions(staker.address)).to.be.equal(2); + + const firstId = (await veVirtual.locks(staker.address, 0)).id; + await veVirtual.connect(staker).convertVeVirtualToRVirtual(firstId); + + expect(await veVirtual.numPositions(staker.address)).to.be.equal(1); + expect(await rVirtual.balanceOf(staker.address)).to.be.equal(parseEther("100")); + }); + + it("should remove voting power on conversion but preserve historical snapshots", async function () { + await veVirtual.connect(staker).delegate(staker.address); + await veVirtual.connect(staker).stake(parseEther("100"), 52, false); + const id = (await veVirtual.locks(staker.address, 0)).id; + + expect(await veVirtual.getVotes(staker.address)).to.be.equal(parseEther("100")); + const blockBeforeConversion = await ethers.provider.getBlockNumber(); + + await veVirtual.connect(staker).convertVeVirtualToRVirtual(id); + + expect(await veVirtual.getVotes(staker.address)).to.be.equal(0); + expect( + await veVirtual.getPastVotes(staker.address, blockBeforeConversion) + ).to.be.equal(parseEther("100")); + }); + + it("should not leave any residual VIRTUAL allowance on veVirtual toward the converter", async function () { + await veVirtual.connect(staker).stake(parseEther("100"), 52, false); + const id = (await veVirtual.locks(staker.address, 0)).id; + await veVirtual.connect(staker).convertVeVirtualToRVirtual(id); + + expect( + await virtual.allowance(veVirtual.target, converter.target) + ).to.be.equal(0); + }); + }); + + describe("forceApprove instead of raw approve (L-04 fix)", function () { + it("should overwrite (not error on) a stale nonzero allowance toward a no-op converter", async function () { + // A no-op converter never pulls the approved VIRTUAL, so the allowance from the + // FIRST conversion call is left standing at the full lock amount. + const noOpConverter = await ethers.deployContract("NoOpConverterMock", [ + virtual.target, + ]); + await veVirtual.setRVirtualConverter(noOpConverter.target); + + await veVirtual.connect(staker).stake(parseEther("100"), 52, false); + const firstId = (await veVirtual.locks(staker.address, 0)).id; + await veVirtual.connect(staker).convertVeVirtualToRVirtual(firstId); + + expect( + await virtual.allowance(veVirtual.target, noOpConverter.target) + ).to.be.equal(parseEther("100")); + + // A SECOND lock/conversion must not revert despite the standing non-zero + // allowance - forceApprove() overwrites it cleanly. (A raw, non-force approve() + // would also succeed on a standard ERC20 like VIRTUAL, but forceApprove is the + // hardened SafeERC20 path that also works on tokens which reject a direct + // nonzero-to-nonzero approve, e.g. USDT-style tokens.) + await virtual.transfer(staker.address, parseEther("50")); + await veVirtual.connect(staker).stake(parseEther("50"), 52, false); + const secondId = (await veVirtual.locks(staker.address, 0)).id; + + await expect( + veVirtual.connect(staker).convertVeVirtualToRVirtual(secondId) + ).to.not.be.reverted; + + // Allowance now reflects only the second call's amount - overwritten, not summed. + expect( + await virtual.allowance(veVirtual.target, noOpConverter.target) + ).to.be.equal(parseEther("50")); + }); + }); +}); diff --git a/test/vevirtual.js b/test/vevirtual.js index da832ee2..18014e31 100644 --- a/test/vevirtual.js +++ b/test/vevirtual.js @@ -25,6 +25,65 @@ describe("veVIRTUAL", function () { veVirtual = await upgrades.deployProxy(Contract, [virtual.target, 104]); }); + it("should reject calling initialize() directly on the raw implementation contract (L-12 fix)", async function () { + const implAddress = await upgrades.erc1967.getImplementationAddress(veVirtual.target); + const implementation = await ethers.getContractAt("veVirtual", implAddress); + + await expect(implementation.initialize(virtual.target, 104)).to.be.reverted; + }); + + it("should preserve all existing proxy state across an upgrade that adds the L-12 constructor", async function () { + // Simulates upgrading an already-live veVirtual proxy (existing user stakes/votes) + // to a new implementation that adds `constructor() { _disableInitializers(); }`. + // Proves the constructor - which only ever runs once, at the NEW implementation's + // own deployment - never touches the proxy's storage. + await virtual.transfer(staker.address, parseEther("1000")); + await virtual.connect(staker).approve(veVirtual.target, parseEther("1000")); + await veVirtual.connect(staker).delegate(staker.address); + await veVirtual.connect(staker).stake(parseEther("300"), 52, false); + const id = (await veVirtual.locks(staker.address, 0)).id; + + const implBefore = await upgrades.erc1967.getImplementationAddress(veVirtual.target); + const balanceBefore = await veVirtual.balanceOf(staker.address); + const votesBefore = await veVirtual.getVotes(staker.address); + + // Upgrade to a fresh deployment of the SAME (current, constructor-having) contract - + // this is exactly what a real upgrade does: deploy a brand-new implementation and + // repoint the existing proxy at it. The new implementation's constructor runs during + // THIS deployment, not against the proxy. + const VeVirtualContract = await ethers.getContractFactory("veVirtual"); + const upgraded = await upgrades.upgradeProxy(veVirtual.target, VeVirtualContract, { + // Force a genuinely new implementation deployment even though the bytecode is + // identical to the current one - a real upgrade always deploys fresh, and this + // is what proves the constructor runs on a NEW address, not the old one. + redeployImplementation: "always", + }); + const implAfter = await upgrades.erc1967.getImplementationAddress(upgraded.target); + + expect(implAfter).to.not.equal(implBefore); + expect(upgraded.target).to.equal(veVirtual.target); // same proxy address throughout + + // All pre-upgrade state survives untouched. balanceOf is intentionally time-decayed + // (see L-11), so it ticks down by a negligible amount over the 1-2 blocks the + // upgrade transaction itself takes to mine - compare with a tight tolerance rather + // than exact equality. getVotes is the raw, undecayed sum and must match exactly. + expect(await upgraded.numPositions(staker.address)).to.be.equal(1); + expect((await upgraded.locks(staker.address, 0)).id).to.be.equal(id); + expect(await upgraded.balanceOf(staker.address)).to.be.closeTo( + balanceBefore, + parseEther("0.001") + ); + expect(await upgraded.getVotes(staker.address)).to.be.equal(votesBefore); + + // The functionality still works post-upgrade. + await expect(upgraded.connect(staker).stake(parseEther("50"), 52, false)).to.not.be + .reverted; + + // And the NEW implementation is independently hardened too. + const newImplementation = await ethers.getContractAt("veVirtual", implAfter); + await expect(newImplementation.initialize(virtual.target, 104)).to.be.reverted; + }); + it("should allow staking", async function () { await virtual.transfer(staker.address, parseEther("1000"));