Description

The scaleVariables function in OstiumVault is called internally when shares are minted or redeemed:

function scaleVariables(uint256 shares, uint256 assets, bool isDeposit) private {
    uint256 supply = totalSupply();

    if (accPnlPerToken < 0) {
        accPnlPerToken =
            accPnlPerToken * int256(supply) / (isDeposit ?
                int256(supply + shares) : int256(supply - shares));
    } else if (accPnlPerToken > 0) {
        totalLiability += int256(shares) * totalLiability / int256(supply)
            * (isDeposit ? int256(1) : int256(-1));
    }

    totalDeposited = isDeposit ?
        totalDeposited + assets : totalDeposited - assets;
}

This scales the accPnlPerToken by the supply in order to ensure that the quantity always maintains a denominator equal to the current supply of tokens.

However, the rescaling only happens if it is less than zero. The zero point is not a special case on the demand side, so if the demand side has unexpectedly high gains, during the time that the vault has a net liability (accPnlPerToken > 0), deposits and withdrawals will not change the denominator. This means that there is a multiplicative factor applied to the variable that is not undone when the protocol restabilizes and accPnlPerToken drops below zero again.

Impact

If there are withdrawals or deposits on the LP side during a time the demand side has temporary high gains such that accPnlPerToken is positive, it will gain an incorrect multiplicative factor.

Recommendations

We recommend maintaining a state variable for total accumulated PNL that is not divided by supply and, when the quantity of acc PNL per supply is required, calculating it on demand. In addition to negating any compounding errors due to precision loss, the statelessness of calculating the quantity on demand also helps simplify the business logic and prevent errors from causing the value to drift over time.

Remediation

Ostium Labs provided the following response:

It is the expected behavior where accPnlPerToken is only updated when the vault is over collateralized to ensure no bank run away happens when the collateralization ratio drops below 100. If accPnlPerToken would be updated during vault’s undercollateralization, at each withdraw the traders losses would be left to the remaining LPs. Moreover, at each deposit, the price would increase at the next epoch leading to exploit opportunities.