Description

Functions in VToken assume the variable totalUnbonding keeps track of the total amount of underlying shares in the unbonding state. However, the rest of the Voyage protocol assumes these variables keep track of the amount of underlying asset in the unbonding state. For example, VToken::pushWithdraw(...) uses shares:

function pushWithdraw(address _user, uint256 _shares) internal {
    unbondings[_user].shares += _shares;
    unbondings[_user].maxUnderlying += convertToAssets(_shares);
    totalUnbonding += _shares;
}

Whereas JuniorDepositToken::totalAssets() assumes the variable expresses an amount of the underlying asset:

contract JuniorDepositToken is VToken {
    function totalAssets() public view override returns (uint256) {
        return asset.balanceOf(address(this)) - totalUnbonding;
    }
}

Impact

This issue has far-reaching consequences, as it influences the amount of assets deposited to both the senior and junior pools. Depending on the exchange rate used to convert between assets and shares, totalUnbonding could become greater or smaller than the correct value.

For example, if convertToAssets(_shares) > _shares then totalUnbonding will be set to a lower-than-intended amount by pushWithdraw(...). This means that Voyage will assume the pool has more assets available than it really does. So, for example, liquidity checks in buyNow(...) will pass when they should not, and purchase orders can mysteriously fail. Additionally, assets locked by lenders for withdraw will still be lent out. This can lead to calls to claim(...) failing and lost assets for lenders.

If convertToAssets(_shares) < _shares then totalUnbonding is instead set to a greater-than-intended value by pushWithdraw(...). Voyage will therefore assume the pool has fewer assets than it really does. Depositor assets will become underutilized by borrowers, and depending on the magnitude of the difference, funds could become effectively locked.

Moreover, since totalUnbonding factors into SeniorDepositToken::totalAssets(...) this can also have an impact on the general accuracy of deposit and withdraw calculations, as the conversion ratio between shares and assets depends on the value returned by totalAssets(...).

Recommendations

Consistently use totalUnbonding to express an amount of assets or an amount of shares.

Assuming the variable is intended to keep track of an amount of assets, at least two modifications to the code would have to be made.

One to VToken::pushWithdraw(...):

function pushWithdraw(address _user, uint256 _shares) internal {
    unbondings[_user].shares += _shares;
    unbondings[_user].maxUnderlying += convertToAssets(_shares);
-   totalUnbonding += _shares;
+   totalUnbonding += convertToAssets(_shares);
}

And another to VToken::claim():

 function claim() external {
     // [...]
     if (availableLiquidity > maxClaimable) {
         // [...]
     } else {
         // [...]
     }
-    totalUnbonding -= transferredShares;
+    totalUnbonding -= convertToAssets(transferredShares);
     asset.safeTransfer(msg.sender, transferredAsset);
}

Remediation

Commits 3320ba3c↗ and acbe5001↗ were indicated as containing remediations for this issue.

Reviewing the remediation for this issue has proven to be challenging due to the pace of the development. A total of 29 commits exist between the commit under review and 3320ba3c↗; the diff between the two commits amounts to 24 solidity files changed, with 324 insertions and 525 deletions. Another 86 commits exist between 3320ba3c↗ and acbe5001↗, with a diff amounting to 29 solidity files changed, 1279 insertions, and 969 deletions.

The two commits appear to correctly fix the issue; we largely based this evaluation on the description of the applied changes provided by the Voyage team due to the considerable amount of changes, which seems compatible with what can be observed in the commits. The totalUnbonding function is not used anymore in the computation of totalAssets; two other functions, totalUnbondingAsset and unbonding, were introduced, respectively computing the amount of assets and shares that are in the unbonding state.