Zellic - Audit Reports

Category: Code Maturity

Drain and financial loss resulting from rounding issue

Medium Severity

Medium Impact

Low Likelihood

Description

In AllocatorVaultV1, the convertToShares and convertToAssets functions determine the ratio between assets and shares, influencing how deposits and withdrawals are executed:

function convertToShares(uint256 assets) public view override returns (uint256) {
    return (assets * (totalSupply() + VIRTUAL_SHARES)) / (totalAssets() + VIRTUAL_ASSETS);
}

/**
 * @dev Converts an amount of shares to an equivalent amount of assets
 * @param shares Amount of shares to convert
 * @return Amount of assets
 */
function convertToAssets(uint256 shares) public view override returns (uint256) {
    return (shares * (totalAssets() + VIRTUAL_ASSETS)) / (totalSupply() + VIRTUAL_SHARES);
}

However, if the balance between these values becomes skewed, rounding errors can occur, potentially allowing an attacker to exploit these calculations. This could lead to an attacker receiving more shares than intended for a given deposit or withdrawing more assets than they should.

If the value of totalAssets() is disproportionally larger than that of totalSupply(), an attacker can manipulate the withdrawal process by passing an _underlying value that causes the return value to become zero due to an issue in rounding. Consequently, while the submitted _underlying value remains unchanged, the return value of convertToShares becomes zero. Thus, the attacker can drain assets without providing any shares in return. Such conditions can manifest when the operator supplies initial liquidity.

function withdraw(
        uint256 _underlying,
        address receiver,
        address owner
    )
        public
        override
        nonReentrant
        returns (uint256)
    {
        uint256 maxUnderlying = maxWithdraw(owner);
        require(_underlying > 0, "Cannot withdraw 0");
        require(_underlying <= maxUnderlying, "Exceeded max withdraw");

        harvest();

        uint256 assets = strategy.convertToShares(_underlying);
        uint256 shares = convertToShares(assets);

        _burn(msg.sender, shares);
        // [...]

If totalSupply() is significantly larger than totalAssets(), calling the redeem function may result in convertToAssets returning zero for a given shares value. In this case, users would have their shares burned without receiving any assets in return. This issue can occur if excessive fees create a severe imbalance between total shares and assets.

function redeem(uint256 shares, address receiver, address owner) public override nonReentrant returns (uint256) {
        uint256 maxShares = maxRedeem(owner);
        require(shares > 0, "Cannot redeem 0");
        require(shares <= maxShares, "Exceeded max redeem");

        harvest();
        uint256 assets = convertToAssets(shares);
        uint256 _underlying = strategy.convertToAssets(assets);

        _burn(owner, shares);
        // [...]

Impact

This vulnerability poses risks such as the draining of assets on a small scale, financial losses for users, and the potential for funds to be frozen. While the drain may be minor, if the share values of a nonstandard ERC-4626 represent substantial worth, it could result in severe asset damage. To enhance the security of StakeKit in light of its scalability, it is crucial to consider these security edge cases.

Recommendations

To address the rounding bug, we recommend adopting the rounding rules utilized in ERC-4626↗.

Ensure that rounding is done in a way that avoids excess minting or burning of shares. For example, rounding down when minting and rounding up when burning is a safer approach. This way, the user never gets more shares than they should (during deposits) or loses fewer shares than they should (during withdrawals).

// ERC4626Upgradeable.sol
    function previewDeposit(uint256 assets) public view virtual returns (uint256) {
        return _convertToShares(assets, Math.Rounding.Floor);
    }

    /** @dev See {IERC4626-previewMint}. */
    function previewMint(uint256 shares) public view virtual returns (uint256) {
        return _convertToAssets(shares, Math.Rounding.Ceil);
    }

    /** @dev See {IERC4626-previewWithdraw}. */
    function previewWithdraw(uint256 assets) public view virtual returns (uint256) {
        return _convertToShares(assets, Math.Rounding.Ceil);
    }

    /** @dev See {IERC4626-previewRedeem}. */
    function previewRedeem(uint256 shares) public view virtual returns (uint256) {
        return _convertToAssets(shares, Math.Rounding.Floor);
    }

Remediation

This issue has been acknowledged by StakeKit, and a fix was implemented in commit e01ab9b9↗.