Virtue V2

Virtue will support multiple collaterals, with IOTA and wBTC likely among the first to be integrated. In the future, the protocol team also plans to add RWA collaterals, including their own RBILL token. However, it remains unclear which price oracle implementation will be used for RBILL, or whether the primary oracle providers for the IOTA ecosystem, Pyth and Supra, can support it. If a custom implementation is required, it must be robust and adhere to the design principles of the PriceFeed contracts, which were built with Chainlink price feeds in mind. This robustness is essential because the PriceFeed contract will trigger a shutdown of its associated branch if it detects an invalid or stale price, potentially affecting the overall system health.

StabilityPool::_triggerVIRTUEIssuance is called when a user deposits VUSD tokens to the stability pool or when the user withdraws tokens from it:

function _triggerVIRTUEIssuance(
    uint256 _boldAmount,
    bool isDebit
) internal {
    ...
    communityIssuance.updateShares(
        address(troveManager),
        msg.sender,
        _boldAmount,
        true,
        isDebit
    );
    communityIssuance.claimRewards(address(troveManager), msg.sender, true);
}

Assume that someone provides N tokens for the first time to the Stability Pool by calling StabilityPool::provideToSP. When the CommunityIssuance::updateShares operation is hit, the user won’t receive any rewards, but the shares of the user will be increased. Since the user's userRewardDebt is calculated based on their previous shares (before the update) it will remain at 0.

function _triggerVIRTUEIssuance(
    uint256 _boldAmount,
    bool isDebit
) internal {
    ...
    communityIssuance.updateShares(
        address(troveManager),
        msg.sender,
        _boldAmount,
        true,
        isDebit
    );
    communityIssuance.claimRewards(address(troveManager), msg.sender, true);
}

function updateShares(
    ...
) external {
    ...
    // @Dedaub: 0 rewards will be accrued
    _handleRewards(branch, user, isDebtRewarder);
    ...
    if (isDebit) {
        uint256 maxReduction = currentShares > MIN_DEBT
            ? currentShares - MIN_DEBT
            : 0;
        uint256 actualReduction = shareChange > maxReduction
            ? maxReduction
            : shareChange;
        rewarder.userShares[user] -= actualReduction;
        rewarder.totalShares -= actualReduction;
    } else {
        rewarder.userShares[user] += shareChange;
        rewarder.totalShares += shareChange;
    }
}

Afterwards, when CommunityIssuance::claimRewards is hit, the pendingRewards value inside CommunityIssuance::_handleRewards will be non-zero, as rewarder.userRewardDebt[user] will be 0:

function _handleRewards(
    ...
) internal {
    // @Dedaub: accrues the rewards per share sum
    updateRewards(branch, isDebtRewarder);

    Rewarder storage rewarder = isDebtRewarder
        ? branchDebtRewarder[branch]
        : branchStabilityPoolRewarder[branch];

    // @Dedaub: Now the user has N shares
    uint256 userShares = rewarder.userShares[user];

    // @Dedaub: userRewardDebt until now is 0
    uint256 pendingRewards = (userShares * rewarder.accRewardsPerShare) /
        1e18 - rewarder.userRewardDebt[user];

    if (pendingRewards > 0) {
        virtueToken.safeTransfer(user, pendingRewards);
        totalVIRTUEIssued += pendingRewards;
        rewarder.virtuesIssued += pendingRewards;
        emit VirtueIssued(user, pendingRewards, isDebtRewarder);
    }

    rewarder.userRewardDebt[user] = (userShares * rewarder.accRewardsPerShare) /
        1e18;
}

Effectively, the user was able to atomically reap the benefits of depositing N tokens. Given that:

• Those N VUSD tokens can be acquired atomically with a flash-loan after depositing the required amount of collateral to the protocol
• The value of N can be appropriately chosen so that for the current rewarder.accRewardsPerShare value, a desired amount of rewards is taken out of the contract

It becomes apparent that an attacker can drain the VIRTUE rewards and potentially profit from selling them and repaying the used flash-loan.

It’s important to note that this attack is enabled because of a subtle point in the protocol’s implementation of the Masterchef algorithm:

function _handleRewards(
    ...
) internal {
    ...
    uint256 userShares = rewarder.userShares[user];
    ...

    rewarder.userRewardDebt[user] = (userShares * rewarder.accRewardsPerShare) /
        1e18;
}

When CommunityIssuance::updateShares is first invoked, at the point where the above snippet is executed:

• The user has not received any shares
• The user’s reward debt is updated using the non-updated number of shares

In typical MasterChef/Synthetix staking implementations, newly deposited tokens should be immediately multiplied by the reward-per-token accumulator and stored as the user’s reward debt. Semantically, this makes all rewards accrued up until the moment the user enters the reward system unclaimable. The user should not receive rewards for deposits they were not part of but will be able to earn rewards thereafter.

VIRTUE is distributed as rewards to borrowers depending on their interactions with the system in BorrowerOperations.sol. This behaviour is implemented through calls to BorrowerOperations::triggerVIRTUEIssuance, which in turn calls CommunityIssuance::updateShares. This updates the shares of the user, and awards any due rewards in VIRTUE by calling CommunityIssuance::_handleRewards.

...
if (pendingRewards > 0) {
    virtueToken.safeTransfer(user, pendingRewards);
    totalVIRTUEIssued += pendingRewards;
    rewarder.virtuesIssued += pendingRewards;
    emit VirtueIssued(user, pendingRewards, isDebtRewarder);
}
...

Since the amount of rewards available is limited, _handleRewards will revert when the user is owed more VIRTUE rewards than the CommunityIssuance contract holds.

This will block any rewardable operations (including adding/withdrawing collateral and repaying/withdrawing VUSD) in all branches, potentially DOSing the system until CommunityIssuance is funded again.

This issue is exacerbated by the fact that currently the accrued rewards per share index continues to be updated indefinitely, even after the distribution period ends. This means that to fully address this issue and maintain the financial incentives of distributing rewards only for a fixed period, the team should both ensure that the contract has a sufficient balance and stop the accrual of rewards in a timely manner.

This is an additional consequence of the misplaced reward debt updated that was noted in C1:

Assume that an existing user performs an operation that will remove CommunityIssuance shares (e.g. remove VUSD from the stability pool by calling StabilityPool::withdrawFromSP). In a similar fashion to C1, CommunityIssuance::updateShares will:

function updateShares(
    ...
) external {
    ...
    // @Dedaub: rewards will be accrued according to current shares
    //         !! reward debt will be updated using current shares as well !!
    _handleRewards(branch, user, isDebtRewarder);
    ...
    if (isDebit) {
        uint256 maxReduction = currentShares > MIN_DEBT
            ? currentShares - MIN_DEBT
            : 0;
        uint256 actualReduction = shareChange > maxReduction
            ? maxReduction
            : shareChange;
        // @Dedaub: shares will be reduced here
        rewarder.userShares[user] -= actualReduction;
        rewarder.totalShares -= actualReduction;
    } else {
        rewarder.userShares[user] += shareChange;
        rewarder.totalShares += shareChange;
    }
}

• Use the current share number to accrue any rewards (inside _handleRewards)
• Use the current share number to update the user’s reward debt (inside _handleRewards) — recall that this should not happen usually
• Appropriately reduce the user’s shares

When CommunityIssuance::claimRewards is hit, the calculation for pendingRewards will underflow because although rewarder.accRewardsPerShare didn’t change, the user’s shares have been reduced and rewarder.userRewardDebt[user] had been set with the old (higher) number of shares

function _handleRewards(
    ...
) internal {
    ...
    uint256 pendingRewards = (userShares * rewarder.accRewardsPerShare) /
        1e18 - rewarder.userRewardDebt[user];
    ...
}

Several upgrades to the Liquity v2 codebase, including some critical ones, have not been merged into the Virtue codebase. More specifically, it appears that commits after the commit with hash 66065d7da32f9474a2a6a3893f55807eac032898, which was committed to the main branch of the Liquity v2 BOLD repository on October 30, have not been merged into the Virtue codebase. As a result, several fixes and improvements to core protocol functionality, such as those related to the PriceFeeds, StabilityPool, TroveManager, and Zappers, are missing.

The protocol aims to enforce a maximum number of created VUSD tokens as a mechanism to control how much of the total debt originates from a particular collateral branch. This is implemented in ActivePool::canOpenTrove

function canOpenTrove(uint256 debtToAdd) public view returns (bool) {
    return
        maxDebtLimit == 0
            ? true
            : this.getBoldDebt() + debtToAdd <= maxDebtLimit;
}

This is mainly aimed at controlling the debt that gets created inside RWA collateral branches by appropriately checking all operations that can increase the debt in the BorrowerOperations contract

function adjustTrove(
    ...
) external override {
    ...
    if (_isDebtIncrease && !activePool.canOpenTrove(_boldChange))
        revert DebtLimitReached();
    }
    ...
}

The issue lies in the fact that a similar check is missing for the following operations that can also increase the system’s debt:

• BorrowerOperation::openTroveAndJoinInterestBatchManager
• BorrowerOperation::withdrawBold

CommunityIssuance is intended to be funded via the CommunityIssuance::setRewardsDistribution function, during which VIRTUE tokens are transferred from the owner to the contract in order to to be distributed to protocol users.

function setRewardsDistribution(
    ...
) external onlyOwner {
    ...
    require(totalBranchWeight == 100, "Branch weights must sum to 100");
    virtueToken.safeTransferFrom(msg.sender, address(this), totalAmount);
    ...
}

Currently, the tokens that are meant to be distributed via the CommunityIssuance contract are minted to the Guardian role:

...
uint communityAllocation = (totalSupply * 60) / 100;
_mint(_guardianAddress, communityAllocation);
...

But it should be noted that during the VIRTUE token’s first year of being deployed, the guardian can only transfer the issued tokens to a lock up contract:

function _transfer(
    address sender,
    address recipient,
    uint256 amount
) internal {
    ...
    if (_callerIsGuardian(sender) && _isFirstYear()) {
        _requireRecipientIsRegisteredLC(recipient);
    }
    ...
}

However, once the tokens are transferred into a lock up contract, they can only be removed once the lock up duration passes, and the lock up duration has to be at least one year after the VIRTUE token’s deployment:

constructor(
    ...
    uint _unlockTime
) {
    ...
    _requireUnlockTimeIsAtLeastOneYearAfterSystemDeployment(_unlockTime);
    ...
}

LockupContracts have no way to approve allowances or call CommunityIssuance::setRewardsDistribution and the locked tokens can only be moved out by calling LockupContract::withdrawVIRTUE:

function withdrawVIRTUE() external {
    _requireCallerIsBeneficiary();
    _requireLockupDurationHasPassed();
    IVIRTUEToken virtueTokenCached = virtueToken;
    uint VIRTUEBalance = virtueTokenCached.balanceOf(address(this));
    virtueTokenCached.transfer(beneficiary, VIRTUEBalance);
    emit LockupContractEmptied(VIRTUEBalance);
}

This means that the only way for the Guardian to transfer the funds is to either wait or have them locked for at least a year. As a result, VIRTUE rewards can only be issued a year after the protocol’s launch.

While this is perfectly fine from a functional standpoint, it may reduce the financial incentives for participating in the protocol. Given that reward distribution is intended to occur over a fixed period, it would make more sense for the rewards to be available at the protocol’s launch rather than a year later.

BorrowerOperations::withdrawBold calls the function _triggerVIRTUEIssuance with the isDebit parameter set to true, which is incorrect as the rest debt increasing operations set isDebit to false, while debt decreasing operations set it to true. As a result, users borrowing via the function withdrawBold will experience a decrease in their CommunityIssuance shares rather than an increase.

The CommunityIssuance contract differentiates reward issuing parameters based on whether the rewarder entity that invokes a reward issuance operation is called by the StabilityPool contract or the BorrowerOperations contract.

...
mapping(address => Rewarder) public branchDebtRewarder;
mapping(address => Rewarder) public branchStabilityPoolRewarder;
...

However, in the case of the StabilityPool contract, the rewarder is always marked to be a debt rewarder, which means that both the StabilityPool and BorrowerOperations contracts end up using the same rewarder parameters.

...
if (address(communityIssuance) == address(0)) {
    return;
}
communityIssuance.updateShares(
    address(troveManager),
    msg.sender,
    _boldAmount,
    true, // @Dedaub: rewarder is marked to be a debt rewarder
    isDebit
);
...

When BorrowerOperations::openTrove is invoked, the owner of the NFT that corresponds to the created trove is inserted into the troveIdOfOwner mapping:

function openTrove(
    ...
) external override returns (uint256) {
    ...
    troveIdOfOwner[vars.troveId] = _owner;

    // Set the stored Trove properties and mint the NFT
    troveManager.onOpenTrove(
        _owner,
        vars.troveId,
        vars.change,
        _annualInterestRate
    );
    ...
}

However:

• The information in the mapping will become invalidated if the corresponding NFT gets transferred
• BorrowerOperations::openTroveAndJoinInterestBatchManager does not update the mapping

There are no apparent security consequences because of this, but anyone reading the public troveIdOfOwner state variable might be consuming inconsistent data.

In the VIRTUE token’s issuance schedule there are multiple parties that receive a percentage of the token’s total supply.

...
_mint(_guardianAddress, communityAllocation);
...
_mint(_teamAddress, teamAllocation);
...
_mint(_ecosystemGrowthAddress, ecosystemGrowthAllocation);
...
_mint(_treasuryAddress, treasuryAllocation);
...
_mint(_fairlaunchAddress, fairlaunchAllocation);
...
_mint(_airdropAddress, airdropAllocation);
...

Some entities (e.g., the Guardian) play a crucial role in appropriately distributing the issued tokens to other parts of the protocol. However, they seem to be conceptualized as EOAs rather than smart contracts whose logic can be verified. In the case of the Guardian role, if the Guardian account is taken over within the first year of the system’s deployment, a malicious actor would be able to create a lock up contract of their own and transfer the tokens there.

In general, the issuance mechanism’s liveness is somewhat correlated with the ownership of the private keys of such trusted entities.

The AccessRegistry contract has been introduced by the Virtue team as a means of controlling access to the special RWA collateral branches.

Since those branches will be geared towards institutional investors, this does not undermine the protocol’s degree of decentralization. However, we highlight the owner’s power to mark additional collateral branches as special and restrict access to them.

CommunityIssuance.setRewardDistribution:

• Line 224 declares an unused variable remainingAmount.
• Rewarder weights are inputted as an array, expecting that the values sum up to 100%. However, only the first two elements are used.
• Checks that distributionPeriod is within the required bounds, however this is only set once (hardcoded in the constructor).

ICurveStableswapNGPool does not declare the function signature for get_dy, although this is expected (see HybridCurveUniV3ExchangeHelpers::80).

CommunityIssuance.updateShares rewards shares to its caller. Troves may have managers that act on the owner’s behalf. When these managers adjust the trove they will be the ones receiving VIRTUE rewards, rather than the trove’s owner directly. If these managers are implemented as smart contracts by the owner this may result in VIRTUE being locked up, should the manager smart contract not implement functions to manage VIRTUE.

The LockupContract constructor does not validate that the _beneficiary address is not equal to address(0). It also does not perform the 0 address check for _virtueTokenAddress.

There exist several unused constants:

• LockupContractFactory::LOCKUPFACTORY_ADMIN
• LockupContractFactory::NAME
• LockupContractFactory::SECONDS_IN_ONE_YEAR
• BorrowerOperations::SCALE_FACTOR

The function LockupContractFactory::isRegisteredLockup can be made external.

There exist several unused storage variables:

• BorrowerOperations::deposits
• BorrowerOperations::depositSnapshots
• BorrowerOperations::P
• BorrowerOperations::currentScale
• BorrowerOperations::currentEpoch
• BorrowerOperations::epochToScaleToSum
• BorrowerOperations::troveIdOfOwner
• BorrowerOperations::epochToScaleToG
• BorrowerOperations::lastVIRTUEError
• CommunityIssuance::accessRegistry
• StabilityPool::epochToScaleToG
• StabilityPool::epochToScaleToSum
• StabilityPool::lastVIRTUEError

There exist several unused structs:

• BorrowerOperations::Deposit
• BorrowerOperations::Snapshots
• StabilityPool::Snapshots::G

CommunityIssuance::setRewardsDistribution loops over branches and for each branch i, loops over rewarderWeights[i], even though rewarderWeights[i] is expected to only contain 2 elements, rendering the loop unnecessary. Additionally, rewarderWeights[i][0] and rewarderWeights[i][1] are used to calculate debtRewardShare and stabilityRewardShare, respectively, by multiplying by branchShare and dividing by 100. Instead, stabilityRewardShare could simply be assigned as branchShare - rewarderWeights[i][0].

The following state state variables are only set during construction and can thus be declared immutable in order to save on gas consumption when the variable is read:

• AccessRegistry::collateralRegistry
• VIRTUEToken::_CACHED_THIS

During construction, the VIRTUE token allocates a fixed percentage of tokens to various protocol entities:

// --- Initial VIRTUE allocations ---
uint totalSupply = _1_MILLION * 100; // 100 million tokens
// Community: 60% of total supply
uint communityAllocation = (totalSupply * 60) / 100;
_mint(_guardianAddress, communityAllocation);
// Team: 20% of total supply
uint teamAllocation = (totalSupply * 20) / 100;
_mint(_teamAddress, teamAllocation);
// Ecosystem Growth: 10% of total supply
uint ecosystemGrowthAllocation = (totalSupply * 10) / 100;
_mint(_ecosystemGrowthAddress, ecosystemGrowthAllocation);
// Treasury: 5% of total supply
uint treasuryAllocation = (totalSupply * 5) / 100;
_mint(_treasuryAddress, treasuryAllocation);
// Fairlaunch: 4% of total supply
uint fairlaunchAllocation = (totalSupply * 4) / 100;
_mint(_fairlaunchAddress, fairlaunchAllocation);
// Airdrop: 1% of total supply
uint airdropAllocation = (totalSupply * 1) / 100;
_mint(_airdropAddress, airdropAllocation);

// Ensure total allocations sum up to the total supply
uint remainingSupply = totalSupply -
    (
        communityAllocation +
        teamAllocation +
        ecosystemGrowthAllocation +
        treasuryAllocation +
        fairlaunchAllocation +
        airdropAllocation
    );

require(
    remainingSupply == 0,
    "VIRTUEToken: Allocations do not match total supply"
);

As the constructor demands that all of the total supply gets allocated to the selected entities, and because all the values of token mints are the result of divisions that get explicitly rounded down, the remainingSupply might be non-zero.

This will happen if the initial total supply changes (before the token gets deployed) and the new total supply does not get divided by the allocation percentage. We expect that the total supply value will not change before deployment, but this could affect future deployments of the token in other networks.

The following snippet in TroveManager::redeemCollateral is loop-invariant and can therefore be moved out of the containing loop in order to save on gas consumption:

while (
    singleRedemption.troveId != 0 &&
    remainingBold > 0 &&
    _maxIterations > 0
) {
    ...
    uint256 branchId = collateralRegistry.getTroveManagerIndex(
        address(this)
    );
    //Skip troves that are in a special branch and the redeemer is not allowed
    if (
        accessRegistry.isSpecialBranch(branchId) &&
        !accessRegistry.hasBranchRedeemAccess(branchId, redeemer)
    ) {
        ...
    }
    ...
}

Since the factory relies on CREATE and not CREATE2, the deployment address of a deployed lockup contract will solely depend on the factory contract's transaction nonce.

function deployLockupContract(
    address _beneficiary,
    uint _unlockTime
) external override returns (LockupContract lockupContract) {
    address virtueTokenAddressCached = virtueTokenAddress;
    _requireVIRTUEAddressIsSet(virtueTokenAddressCached);
    lockupContract = new LockupContract(
        virtueTokenAddressCached,
        _beneficiary,
        _unlockTime
    );
    lockupContractToDeployer[address(lockupContract)] = msg.sender;
    emit LockupContractDeployedThroughFactory(
        address(lockupContract),
        _beneficiary,
        _unlockTime,
        msg.sender
    );
}

Assuming that:

• Action A: A user deploys a lockup contract by calling deployLockupContract in one transaction ( action A )
• Action B: The same user or another entity uses the address of action A in some way (e.g., the lockup address could be used as the recipient of a VIRTUE token transfer)

If Action A and Action B are sent simultaneously as separate transactions, then someone else might create a separate Action C that invokes LockupContractFactory::deployLockupContract with a different _beneficiary parameter and attempt to have the transactions ordered as:

• Action C
• Action A
• Action B

Due to the ordering, Action B will use the front-runner’s lockup contract instead of the one created by Action A.

This is not a probable scenario, and it becomes even harder to execute within the IOTA EVM as validators do not control the ordering of the transactions within a block (unless more than ⅔ of them act maliciously). However, this might come into play if the protocol ever deploys on other chains as well.

The code is compiled with Solidity 0.8.28. At the time of writing, version 0.8.28 has no known bugs.