lemonmon - Potential accounting problems due to issue in `ClearingHouse.updatePositions()`

[sherlock-admin]

lemonmon

high

Potential accounting problems due to issue in ClearingHouse.updatePositions()

Summary

Potential issue in ClearingHouse.updatePositions() when lastFundingTime is not being updated by ClearingHouse.settleFunding.

Vulnerability Detail

ClearingHouse.lastFundingTime is only updated, when _nextFundingTime is not zero:

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/ClearingHouse.sol#L281-L282

_nextFundingTime is determined a few lines above by a call to amms[i].settleFunding():

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/ClearingHouse.sol#L267

The return value of amms[i].settleFunding() can be zero for _nextFundingTime, if the block.timestamp is smaller than the next funding time:

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/AMM.sol#L236-L249

That means that if the last market inside the amms array has not reached the next funding time at block.timestamp, _nextFundingTime will be zero and lastFundingTime will not be updated.

Then when ClearingHouse.updatePositions() is called, it will not process fundingPayment because lastFundingTime was not updated:

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/ClearingHouse.sol#L241-L250

Impact

Unrealized funding payments are not settled correctly, potentially leading to incorrect margin accounting when opening a position. Also marginAccount.realizePnL() (line 255 ClearingHouse.sol) won't get called, so the trader won't receive funds that they should receive.

Note: ClearingHouse.updatePositions() is called by ClearingHouse._openPosition (line 141 ClearingHouse.sol).

Note: ClearingHouse.liquidate -> ClearingHouse.openPosition -> ClearingHouse._openPosition

There can be multiple potential issues with accounting that can result due to this issue, both when liquidating and when opening a position.

Code Snippet

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/ClearingHouse.sol#L281-L282

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/ClearingHouse.sol#L267

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/AMM.sol#L236-L249

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/ClearingHouse.sol#L241-L250

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/ClearingHouse.sol#L140-L141

Tool used

Manual Review

Recommendation

Consider adjusting the code inside ClearingHouse.settleFunding() to account for the case where the last market inside the amms array returns zero for _nextFundingTime when Amm.settleFunding() is called (line 267 ClearingHouse.sol). For example introduce a boolean variable that tracks whether a market inside amms array didn't return zero for _nextFundingTime.

// ClearingHouse
// settleFunding
268            if (_nextFundingTime != 0) {
269                _marketReachedNextFundingTime = true; // <-- @audit new boolean to track

281        if (_marketReachedNextFundingTime) {
282            lastFundingTime = _blockTimestamp();
283        }

[asquare08]

All amms will have the same nextFundingTime as amm.settleFunding is called together for all amms in a single tx. So _nextFundingTime will either be 0 or none for all amms.

    function settleFunding() override external onlyDefaultOrderBook {
        uint numAmms = amms.length;
        uint _nextFundingTime;
        for (uint i; i < numAmms; ++i) {
            int _premiumFraction;
            int _underlyingPrice;
            int _cumulativePremiumFraction;
            (_premiumFraction, _underlyingPrice, _cumulativePremiumFraction, _nextFundingTime) = amms[i].settleFunding();
            if (_nextFundingTime != 0) {
                emit FundingRateUpdated(
                    i,
                    _premiumFraction,
                    _underlyingPrice.toUint256(),
                    _cumulativePremiumFraction,
                    _nextFundingTime,
                    _blockTimestamp(),
                    block.number
                );
            }
        }
        // nextFundingTime will be same for all amms
        if (_nextFundingTime != 0) {
            lastFundingTime = _blockTimestamp();
        }
    }

[ctf-sec]

Closed this issue based on sponsor's comment,

the watson is welcome to escalate with a more clear impact and more proof

[lemonmon1984]

Escalate

All amms will have the same nextFundingTime as amm.settleFunding is called together for all amms in a single tx.

It is not neccessarily true for certain conditions, in which ClearingHouse.whitelistAmm() is involved. One possible scenario would be:

1. A couple of AMMs are whitelisted via ClearingHouse.whitelistAmm() where a nextFundingTime is assigned to them.
2. After the nextFundingTime was reached, ClearingHouse.whitelistAmm() is called again to add another AMM.
3. Then the ClearingHouse.settleFunding() is called and returns 0 for the last AMM that was just added in the previous step, because its nextFundingTime is in the future and ClearingHouse.sol line 248 is true and returns 0 for nextFundingTime, which then leads to the issue described here.

Note: ClearingHouse.whitelistAmm() and ClearingHouse.settleFunding() is likely to be initiated by separate entities. Therefore, the order of transactions is not guaranteed. Also the tx execution can be delayed due to network congestions. That's why this scenario may occur.

Example:

fundingPeriod: 3h call ClearingHouse.whitelistAmm(): at 16:00 (block.timestamp)

then AMM.startFunding() gets called l521 in ClearingHouse.sol here it calculates: nextFundingTime = ((16 + 3) / 3) * 3 = 18

at 19:00 another AMM is added: call ClearingHouse.whitelistAmm(): at 19:00 (block.timestamp) then AMM.startFunding() gets called l521 in ClearingHouse.sol here it calculates: nextFundingTime = ((19 + 3) / 3) * 3 = 21

then also at 19:00 after another AMM was just added: call ClearingHouse.settleFunding() which calls AMM.settleFunding, and the last index of AMMs contains the added AMM from 19:00 with nextFundingTime 21 so AMM.settleFunding will return 0,0,0,0 l249 because nextFundingTime is in the future. This leads to exactly the issue described that then on line 281 in ClearingHouse.sol _nextFundingTime will be 0.

Other scenario

In another scenario, if the ClearingHouse.settleFunding() is called past the nextFundingTime due to network congestion, the nextFundingTime will shift from the fixed schedule. While it is shifted, if the ClearingHouse.whitelistAmm() is called, then it gives a certain window for the ClearingHouse.settleFunding() call will face the same issue.

[sherlock-admin2]

Escalate

All amms will have the same nextFundingTime as amm.settleFunding is called together for all amms in a single tx.

It is not neccessarily true for certain conditions, in which ClearingHouse.whitelistAmm() is involved. One possible scenario would be:

1. A couple of AMMs are whitelisted via ClearingHouse.whitelistAmm() where a nextFundingTime is assigned to them.
2. After the nextFundingTime was reached, ClearingHouse.whitelistAmm() is called again to add another AMM.
3. Then the ClearingHouse.settleFunding() is called and returns 0 for the last AMM that was just added in the previous step, because its nextFundingTime is in the future and ClearingHouse.sol line 248 is true and returns 0 for nextFundingTime, which then leads to the issue described here.

Note: ClearingHouse.whitelistAmm() and ClearingHouse.settleFunding() is likely to be initiated by separate entities. Therefore, the order of transactions is not guaranteed. Also the tx execution can be delayed due to network congestions. That's why this scenario may occur.

Example:

fundingPeriod: 3h call ClearingHouse.whitelistAmm(): at 16:00 (block.timestamp)

then AMM.startFunding() gets called l521 in ClearingHouse.sol here it calculates: nextFundingTime = ((16 + 3) / 3) * 3 = 18

at 19:00 another AMM is added: call ClearingHouse.whitelistAmm(): at 19:00 (block.timestamp) then AMM.startFunding() gets called l521 in ClearingHouse.sol here it calculates: nextFundingTime = ((19 + 3) / 3) * 3 = 21

then also at 19:00 after another AMM was just added: call ClearingHouse.settleFunding() which calls AMM.settleFunding, and the last index of AMMs contains the added AMM from 19:00 with nextFundingTime 21 so AMM.settleFunding will return 0,0,0,0 l249 because nextFundingTime is in the future. This leads to exactly the issue described that then on line 281 in ClearingHouse.sol _nextFundingTime will be 0.

Other scenario

In another scenario, if the ClearingHouse.settleFunding() is called past the nextFundingTime due to network congestion, the nextFundingTime will shift from the fixed schedule. While it is shifted, if the ClearingHouse.whitelistAmm() is called, then it gives a certain window for the ClearingHouse.settleFunding() call will face the same issue.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[asquare08]

A good corner case is described. It's a valid issue and can be marked as medium severity

[IAm0x52]

Agreed with escalation, valid corner case

[hrishibhat]

Result: Medium Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• lemonmon1984: accepted

[asquare08]

Whitelist amm is governance function, we'll take care that when whitelisting new amm, all amms should have same nextFundingTime