code-423n4 / 2022-12-tigris-findings

8 stars 4 forks source link

Error in trade accumulated interest calculation #628

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Position.sol#L41-L65

Vulnerability details

Impact

The trade's accumulated interest may result smaller than expected when long open interest is zero.

Proof of Concept

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Position.sol#L41-L65

function trades(uint _id) public view returns (Trade memory) {
    ...
    int256 _pendingFunding;
    if (_trade.direction && longOi[...][...] > 0) {
        _pendingFunding = (int256(block.timestamp-lastUpdate[...][...])*fundingDeltaPerSec[...][...])*1e18/int256(longOi[...][...]);
        if (longOi[...][...] > shortOi[...][...]) {
            _pendingFunding = -_pendingFunding;
        } else { // NOTE else shortOi > longOi
            _pendingFunding = _pendingFunding*int256(1e10-vaultFundingPercent[_trade.asset][_trade.tigAsset])/1e10;
        }
    } else if (shortOi[_trade.asset][_trade.tigAsset] > 0) { //
        _pendingFunding = (int256(block.timestamp-lastUpdate[...][...])*fundingDeltaPerSec[...][...])*1e18/int256(shortOi[...][...]);
        if (shortOi[...][...] > longOi[...][...]) { // shortOi > longOi
            _pendingFunding = -_pendingFunding;
        } else {
            _pendingFunding = _pendingFunding*int256(1e10-vaultFundingPercent[...][...])/1e10;
        }
    }
    _trade.accInterest += (int256(_trade.margin*_trade.leverage/1e18)*(accInterestPerOi[...][...][...]+_pendingFunding)/1e18)-initId[_id];

    return _trade;
}

The if at line 47 in Position.sol is an and condition between the direction of the trade and the long open interest for the asset/tig asset pair, so if the long open interest is zero, the else branch at line 54 will be executed no matter what is the direction of the trade. As a result, the _pendingFunding value will be nonzero and added to the _trade.accInterest at line 62. Note that the inverse condition is not true, i.e. if direction is short and short open interest is zero then _pendingFunding is zero.

In conclusion if trade direction is true and long open interest is zero, the trade is losing interest because the calculated _pendingFunding for short open interest is then sign inverted and added to the _trade.accInterest at line line 62.

Tools Used

Manual review

Recommended Mitigation Steps

if (_trade.direction && longOi[...][...] > 0) {
    ...
} else if (!_trade.direction && shortOi[_trade.asset][_trade.tigAsset] > 0) {
    ...
}

or

if (_trade.direction) {
    if(longOi[...][...] > 0) {
        ...
    }
} else {
    if(shortOi[_trade.asset][_trade.tigAsset] > 0) {
        ...
    }
}
TriHaz commented 1 year ago

A long position can't be opened while longOi is zero.

c4-sponsor commented 1 year ago

TriHaz marked the issue as sponsor disputed

GalloDaSballo commented 1 year ago

Per disussion on #504 We are missing proof here as well

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof