Margin can't be totally removed in `removeMargin`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Trading.sol#L404-L441

Vulnerability details

Margin can't be totally removed in removeMargin

Impact

Margin can't be removed totally because of revert

Proof of Concept

As we can see in the added tag @audit line 1: uint256 _newMargin = _trade.margin - _removeMargin; //@audit line 1 What means that _newMargin = to actual trade.margin - _removeMargin, that is the amount of margin to remove. This shouldn't be an issue, but the problem is that divisions by 0 revert, if we try to removed all the margin: uint256 _newLeverage = _trade.margin * _trade.leverage / _newMargin;//@audit line 2 We can see that it will divide by _newMargin that is 0, and therefore, removing all the margin is not possible.

Code:

 function removeMargin(
        uint256 _id,
        address _stableVault,
        address _outputToken,
        uint256 _removeMargin,
        PriceData calldata _priceData,
        bytes calldata _signature,
        address _trader
    )
        external
    {
        _validateProxy(_trader);
        _checkOwner(_id, _trader);
        _checkVault(_stableVault, _outputToken); 
        IPosition.Trade memory _trade = position.trades(_id); 
        if (_trade.orderType != 0) revert(); //IsLimit 
        IPairsContract.Asset memory asset = pairsContract.idToAsset(_trade.asset); 

        uint256 _newMargin = _trade.margin - _removeMargin; //@audit line 1
        uint256 _newLeverage = _trade.margin * _trade.leverage / _newMargin;//@audit line 2 

        if (_newLeverage > asset.maxLeverage) revert("!lev"); 
        (uint _assetPrice,) = tradingExtension.getVerifiedPrice(_trade.asset, _priceData, _signature, 0);
        (,int256 _payout) = TradingLibrary.pnl(_trade.direction, _assetPrice, _trade.price, _newMargin, _newLeverage, _trade.accInterest);
        unchecked {
            if (_payout <= int256(_newMargin*(DIVISION_CONSTANT-liqPercent)/DIVISION_CONSTANT)) revert LiqThreshold();
        }
        position.modifyMargin(_trade.id, _newMargin, _newLeverage); 
        _handleWithdraw(_trade, _stableVault, _outputToken, _removeMargin);
        emit MarginModified(_trade.id, _newMargin, _newLeverage, false, _trader);
    }

Recommended mitigation steps

Add an if path to handle case where margin is removed to 0 by calling removeMargin

[TriHaz]

That would happen only if a user tried to remove all margin, which would revert anyway here: https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Trading.sol#L436

No impact so will dispute.

[c4-sponsor]

TriHaz marked the issue as sponsor disputed

[GalloDaSballo]

With the information that I have, in lack of a POC, I believe that rightfully the margin of a position cannot be set to 0 (otherwise you're getting a position with infinite leverage / a position with a payout but no cost)

For this reason, with the sponsor disputing and not much else to use to defend the report, am closing for lack of proof.

I highly recommend adding a coded POC next time to have your report withstand basic scrutiny

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof