Description

During the process of closing out a trade, if a trader made a profit, OstiumVault.sendAssets is called to send them assets.

In OstiumVault, the accPnlPerToken variable slowly (through averaging each epoch) follows the return value of OstiumOpenPnl.getOpenPnl, which is the total PNL of open trades owed to traders. So, assuming a steady state, before the position close, accPnlPerToken has a component corresponding to the amount that would be sent to the trader if their position closed.

However, during sendAssets, this variable is increased, and if it exceeds the maximum, the send reverts:

function sendAssets(uint256 assets, address receiver) external onlyCallbacks {
    // [...]
    int256 accPnlDelta = int256(assets.mulDiv(PRECISION_6, totalSupply(),
        MathUpgradeable.Rounding.Up)); 

    accPnlPerToken += accPnlDelta;
    if (accPnlPerToken > int256(maxAccPnlPerToken())) {
        revert NotEnoughAssets();
    }

The closure of the position immediately decreases the return value of OstiumOpenPnl.getOpenPnl by the PNL, since OstiumOpenPnl.accClosedPnl immediately increases. However, due to the time averaging, this is not reflected in accPnlPerToken yet, so the full PNL of the trade is already in the variable. Adding the PNL of the trade means the PNL is temporarily double counted until the decrease in accPnlPerToken brings it back down to counting it once.

Impact

When maxAccPnlPerToken is constrained, large trades can fail to close with NotEnoughAssets even though there are enough assets to send out, due to an undue double counting of the position size.

Recommendations

We recommend reworking how accClosedPnl works so that when a position is closed, the effect of that close does not have to go through averaging to get reintroduced into the accPnlPerToken summation, because it is already in the variable.

Remediation