shaka - Accounting error for `marginDepositedTotal` in `settleFundingFees()`

[sherlock-admin2]

shaka

high

Accounting error for marginDepositedTotal in settleFundingFees()

Summary

A logic error in the FlatcoinVault:settleFundingFees function can cause accounting errors in the margin deposited total that lead to DoS and loss of funds.

Vulnerability Detail

FlatcoinValut:settleFundingFees is a public function called from multiple places and is used to settle the funding fees between the leverage traders and the LPs.

This function updates, among other things, the _globalPositions.marginDepositedTotal and stableCollateralTotal variables, increasing or decreasing them depending on the sign of the funding fees. When the funding fees are positive, the margin deposited total is increased and the stable collateral total is decreased, and vice versa.

However, there is an error in the ternary operator of line 232. As we can see, when the funding fees are higher than the margin deposited total, they are added, and when they are equal or lower, the margin deposited total is set to 0.

File: FlatcoinVault.sol

232        _globalPositions.marginDepositedTotal = (int256(_globalPositions.marginDepositedTotal) > _fundingFees)
233            ? uint256(int256(_globalPositions.marginDepositedTotal) + _fundingFees)
234            : 0;
235
236        _updateStableCollateralTotal(-_fundingFees);

This is clearly wrong, as having a negative funding fee with a higher absolute value than the margin deposited total would result in an underflow and when the funding fee is greater than the margin deposited total, the margin deposited total is set to 0 instead of being increased by the funding fee.

Impact

When the funding fees are negative and their absolute value is greater than the margin deposited total, the transaction will revert due to an underflow error. Given that this function is called on order announcement, order execution, and liquidation, this will cause the protocol to stop functioning and the funds to be locked.

When the funding fees are greater than the margin deposited total, the margin deposited total will be set to 0 and thus lost, causing the users to lose their funds.

Code Snippet

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

Proof of concept

PoC deposited margin lost

Add the following test to `FundingMathTest` and run `forge test --mt testDepositedMarginLost -vv`: ```solidity function testDepositedMarginLost() public { vm.startPrank(alice); int256 stableDeposit = 100e18; int256 margin = 50e18; int256 additionalSize = 60e18; uint256 collateralPrice = 1000e8; announceAndExecuteDeposit({ traderAccount: alice, keeperAccount: keeper, depositAmount: uint256(stableDeposit), oraclePrice: collateralPrice, keeperFeeAmount: 0 }); announceAndExecuteLeverageOpen({ traderAccount: alice, keeperAccount: keeper, margin: uint256(margin), additionalSize: uint256(additionalSize), oraclePrice: collateralPrice, keeperFeeAmount: 0 }); FlatcoinStructs.GlobalPositions memory globalPositionsStart = vaultProxy.getGlobalPositions(); uint256 stableCollateralTotalStart = vaultProxy.stableCollateralTotal(); console2.log("\n**** Before settling funding fees ****"); console2.log("stableCollateralTotal", stableCollateralTotalStart); console2.log("marginDepositedTotal ", globalPositionsStart.marginDepositedTotal); skip(8 days); int256 fundingFees = vaultProxy.settleFundingFees(); FlatcoinStructs.GlobalPositions memory globalPositionsEnd = vaultProxy.getGlobalPositions(); uint256 stableCollateralTotalEnd = vaultProxy.stableCollateralTotal(); console2.log("\n**** After settling funding fees ****"); console2.log("fundingFees ", fundingFees); console2.log("stableCollateralTotal", stableCollateralTotalEnd); console2.log("marginDepositedTotal ", globalPositionsEnd.marginDepositedTotal); } ``` Console output: ```js Running 1 test for test/unit/Funding-Math/FundingMath.t.sol:FundingMathTest [PASS] testDepositedMarginLost() (gas: 1455844) Logs: **** Before settling funding fees **** stableCollateralTotal 100000000000000000000 marginDepositedTotal 50000000000000000000 **** After settling funding fees **** fundingFees 57601666666666666560 stableCollateralTotal 42398333333333333440 marginDepositedTotal 0 Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.91ms ``` As we can see, when the funding fees (57.6e18) are higher than the margin deposited total (50e18), they are correctly subtracted from the stable collateral total, but they are not added to the margin deposited total. What is more, the margin deposited total is set to 0.

PoC underflow

Add the following test to `FundingMathTest` and run `forge test --mt testRevertUnderflow -vv`: ```solidity function testRevertUnderflow() public { vm.startPrank(alice); int256 stableDeposit = 100e18; int256 margin = 5e18; int256 additionalSize = 120e18; uint256 collateralPrice = 1000e8; announceAndExecuteDeposit({ traderAccount: alice, keeperAccount: keeper, depositAmount: uint256(stableDeposit), oraclePrice: collateralPrice, keeperFeeAmount: 0 }); announceAndExecuteLeverageOpen({ traderAccount: alice, keeperAccount: keeper, margin: uint256(margin), additionalSize: uint256(additionalSize), oraclePrice: collateralPrice, keeperFeeAmount: 0 }); skip(2 days); announceAndExecuteDeposit({ traderAccount: bob, keeperAccount: keeper, depositAmount: uint256(stableDeposit), oraclePrice: collateralPrice, keeperFeeAmount: 0 }); } ``` Console output: ```js Running 1 test for test/unit/Funding-Math/FundingMath.t.sol:FundingMathTest [FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] testRevertUnderflow() (gas: 2202435) Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 11.72ms ```

Tool used

Manual Review

Recommendation

File: FlatcoinVault.sol

        // In the worst case scenario that the last position which remained open is underwater,
        // we set the margin deposited total to 0. We don't want to have a negative margin deposited total.
-       _globalPositions.marginDepositedTotal = (int256(_globalPositions.marginDepositedTotal) > _fundingFees)
+       _globalPositions.marginDepositedTotal = (int256(_globalPositions.marginDepositedTotal) > -_fundingFees)
            ? uint256(int256(_globalPositions.marginDepositedTotal) + _fundingFees)
            : 0;

Duplicate of #195

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: high(2)