Description

In general, the ERC4626 vault uses the current ratio of total shares to total assets for pricing conversion from assets to shares for deposits and conversions from shares to assets for withdrawals.

The VToken vault in Voyage implements a novel two-step withdrawal process.

Users first call withdraw(…), which calls pushWithdraw(…), to record the number of shares being withdrawn and the corresponding value in asset terms and to reserve the total amount of assets being withdrawn by updating totalUnbonding. In the current implementation, burn(…) occurs before this call is made:

shares = previewWithdraw(_amount); // No need to check for rounding error, previewWithdraw rounds up.
if (msg.sender != _owner) {
    _spendAllowance(_owner, msg.sender, shares);
}
beforeWithdraw(_amount, shares);
_burn(_owner, shares);
pushWithdraw(_owner, shares);

This inadvertently alters the total shares and hence the conversion from shares to assets that occurs in pushWithdraw(…):

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

Users then call claim(…) in order to receive their funds. For the case where availableLiquidity > maxClaimable, the incorrect conversion from the previous step will carry over. Furthermore, if availableLiquidity <= maxClaimable, another conversion will also be based on an incorrect total shares:

if (availableLiquidity > maxClaimable) {
    transferredAsset = maxClaimable;
    transferredShares = unbondings[msg.sender].shares;
    resetUnbondingPosition(msg.sender);
} else {
    transferredAsset = availableLiquidity;
    uint256 shares = convertToShares(availableLiquidity);
    reduceUnbondingPosition(shares, transferredAsset);
    transferredShares = shares;
}

Impact

Calling deposit(...) and withdraw(...) in the same transaction repeatedly can lead to draining of the tranches.

For example, in general, let deposit of assets of amount equal to assetDeposited result in an amount of shares equal to sharesReceived being sent to the depositor.

It is expected behavior (and has been verified) that an immediate call (same transaction) to withdraw(...) made with assetDeposited will set the amount of shares to be burned as sharesReceived:

function withdraw(
    uint256 _amount,
    address _receiver,
    address _owner
) public override(ERC4626, IERC4626) returns (uint256 shares) {
    shares = previewWithdraw(_amount); // No need to check for rounding error, previewWithdraw rounds up.

    beforeWithdraw(_amount, shares);

    _burn(_owner, shares);
    pushWithdraw(_owner, shares);

    emit Withdraw(msg.sender, _receiver, _owner, _amount, shares);

The call to _burn(...) in withdraw(...) reduces the totalSupply of shares by sharesReceived so that the call to pushWithdraw(...) overprices the asset when calculating the amount of asset owed to the depositor in unbondings[_user].maxUnderlying += convertToAssets(_shares) and also reserves the assets for withdraw in totalUnbonding += convertToAssets(_shares):

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

The call to convertToAssets(_shares) necessarily overprices the asset. We have fully proven this mathematically, but there is a sufficiently strong intuitive argument.

The price of shares in units of assets is based on the ratio of the balance of assets to shares. From the base implementation of ERC4626 we have

function convertToAssets(uint256 shares)
    public
    view
    virtual
    returns (uint256)
{
    uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

    return supply == 0 ? shares : shares.mulDivDown(totalAssets(), supply);
}

Therefore, if the supply is reduced by a premature _burn(...), we necessarily overinflate the amount of assets the depositor can withdraw. This allows an attacker to drain the funds from the vaults through repeated atomic deposit(...) + withdraw(...) transactions.

Recommendations

Move share burning until the end of claim(...) as suggested below:

    if (availableLiquidity > maxClaimable) {
        transferredAsset = maxClaimable;
        transferredShares = unbondings[msg.sender].shares;
        resetUnbondingPosition(msg.sender);
    } else {
        transferredAsset = availableLiquidity;
        uint256 shares = convertToShares(availableLiquidity);
        reduceUnbondingPosition(shares, transferredAsset);
        transferredShares = shares;
    }
    totalUnbonding -= transferredAsset;
    asset.safeTransfer(msg.sender, transferredAsset);
    _burn(_owner, transferredShares);
}

This positioning should also avoid conflicts with other processes in Voyage.

Remediation

Voyage has moved the call to _burn so that it occurs after the reduction in the unbonding position in claim in commit 63099db1↗. This aligns the implementation with the intended design and avoids overvaluing the assets in the preview and conversion functions.