The long and short open interests can be larger than the max open interest limit

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/PairsContract.sol#L139-L143

Vulnerability details

Impact

The max open interest limit updating function setMaxOi didn't verify the new max open interest limit against existing long/short open interests. The consequence is that the existing assets' long/short open interests may exceed the reduced max open interest limit. This breaks the business logic: the max open interest limit is not supposed to be exceeded.

Proof of Concept

In the test file: 02.PairsContract.js, add below test in Section describe('Protocol-only functions', function () :

    it('Reduce max open interest limit to lower than short/long open interest', async function () {
        //@audit added
        await pairscontract.connect(owner).addAsset(99, 'XYZ/ABC', ethers.constants.AddressZero, ethers.utils.parseEther('1'), ethers.utils.parseEther('100'), 1e10, 3e9);
        await pairscontract.connect(owner).setProtocol(NewTrading.address);
        await pairscontract.connect(owner).setMaxOi(99, StableToken.address, ethers.utils.parseEther('2000'));
        await pairscontract.connect(NewTrading).modifyLongOi(99, StableToken.address, true, ethers.utils.parseEther('500'));
        await pairscontract.connect(NewTrading).modifyShortOi(99, StableToken.address, true, ethers.utils.parseEther('2000'));
        let [longOi, shortOi] = await pairscontract.idToOi(99, StableToken.address);
        expect(longOi).to.equal(ethers.utils.parseEther('500'));
        expect(shortOi).to.equal(ethers.utils.parseEther('2000'));
        await expect(pairscontract.connect(owner).setMaxOi(99, StableToken.address, ethers.utils.parseEther('100'))).not.to.be.reverted;
    });

Tools Used

N/A.

Recommended Mitigation Steps

One of the possible solution is to limit the max open interest limit to the maximum value of the existing assets' long/short open interests; or to set the max open interest limit as a constant.

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Marginally better, but suspiciously similar to it's dups

[TriHaz]

I don't see any issue in that, if max OI is modified, the expected behaviour will be that no new positions will be opened until current OI decreases to less than the new max OI.

[c4-sponsor]

TriHaz marked the issue as sponsor disputed

[GalloDaSballo]

The finding shows how the current value of OI can be above the max if the value for max is reduced.

The sponsor says that this will not cause issues as no new position will be opened.

If no liquidation nor MEV advantageous scenario can be created, I agree with the Sponsor and may keep the finding as QA at most
If an advantageous scenario can be identified I'll rate Med due to conditionality

Will dig deeper

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as duplicate of #377

[c4-judge]

GalloDaSballo marked the issue as satisfactory