_globalPositions.marginDepositedTotal can underflow

Summary

The _globalPositions.marginDepositedTotal could underflow.

Vulnerability Detail

This is very similar to a finding that I reported but has a different cause.

Let's take a look at a settleFundingFees function. This function is used to settle the currently unrecorded funding fees and update the _globalPositions.marginDepositedTotal.

function settleFundingFees() public returns (int256 _fundingFees) {
    ...
    _fundingFees = PerpMath._accruedFundingTotalByLongs(_globalPositions, unrecordedFunding);
    ...
    _globalPositions.marginDepositedTotal = (int256(_globalPositions.marginDepositedTotal) > _fundingFees)
        ? uint256(int256(_globalPositions.marginDepositedTotal) + _fundingFees)
        : 0;
    ...
}

In an edge case where funding fees haven't been settled for long, this can produce an incorrect state. First, we need to look at the _accruedFundingTotalByLongs function. By looking at a function we can see that the result can be either positive or negative based on the unrecodedFunding.

If the unrecordedFunding < 0 this means that shorts have to pay to the longs and if unrecordedFunding > 0 the longs have to pay to the shorts.

When unrecordedFunding > 0 this means profit for the shorts and the return value of the function is negative.

function _accruedFundingTotalByLongs(
    FlatcoinStructs.GlobalPositions memory globalPosition,
    int256 unrecordedFunding
) internal pure returns (int256 accruedFundingLongs) {
    return -int256(globalPosition.sizeOpenedTotal)._multiplyDecimal(unrecordedFunding);
}

Consider the following scenario:

_globalPositions.marginDepositedTotal = 1 ether
_fundingFees = -1.1 ether => it should go to the shorts so we subtract the amount from the longs

Since _fundingFees are negative it means they have to be subtracted from the marginDepositedTotal as the fee for the shorts. Since marginDepositedTotal > _fundingFees the marginDepositedTotal will underflow because

_globalPositions.marginDepositedTotal = uint256(int256(_globalPositions.marginDepositedTotal) + _fundingFees)

If we plug in the numbers = uint256(int256(1 ether) - 1.1 ether) => the problem is that the intermediate calculation results into int256 and is unsafely cast to uint256. Even with a new solidity version >0.8.0 this can underflow.

Try it out in remix. The return value underflows.

// SPDX-License-Identifier: GPL-3.0

pragma solidity 0.8.18;

contract TestContract {
    function _proportionalElapsedTime() public pure returns (uint256) {
        uint256 marginDepositedTotal = 1 ether;
        int256 _fundingFees = -1.1 ether;

        marginDepositedTotal = (int256(marginDepositedTotal) > _fundingFees)
            ? uint256(int256(marginDepositedTotal) + _fundingFees)
            : 0;

        return marginDepositedTotal;
    }
}

Impact

marginDepositedTotal could underflow.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L216-L237

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/libraries/PerpMath.sol#L219-L224

Tool used

Manual Review

Recommendation

Instead of directly casting to uint256 use a safe cast that detects the underflow and accordingly sets the value to 0 instead.

Duplicate of #195

sherlock-audit / 2023-12-flatmoney-judging

evmboi32 - _globalPositions.marginDepositedTotal can underflow #154