Open sherlock-admin3 opened 4 months ago
1 comment(s) were left on this issue during the judging contest.
santipu_ commented:
Medium
Yes, it looks like when marginDepositedTotal
is negative, opening a new leverage position can break an invariant because of setting it to 0 on updateGlobalPositionData
.
By removing this line (not zeroing a negative marginDepositedTotal
), the system is able to handle the scenario.
Escalate
I believe this issue to be medium severity due to the low probability of happening.
For this bug to be triggered it needs the value of globalPositions.marginDepositedTotal
to be negative. These scenarios can cause it:
Scenario 1 is highly unlikely to happen because insolvent positions would have been liquidated before their debt became bigger than the total deposited margin on the market. Moreover, in a volatile market, the protocol team can adjust the value of liquidationBufferRatio
to ensure that even sharp price movements don't cause positions to get underwater.
Scenario 2 is also highly unlikely to happen because when the funding fee is so big, it's expected that the market will regulate itself, bringing the skew back to zero and decreasing the funding fees. Even if the funding fee starts to rapidly decrease the margins of the positions, liquidators will step up and liquidate those underwater positions before bad debt is accrued.
Given the low probability of this bug happening, I think it deserves medium severity.
Escalate
I believe this issue to be medium severity due to the low probability of happening.
For this bug to be triggered it needs the value of
globalPositions.marginDepositedTotal
to be negative. These scenarios can cause it:
- The total debt of the positions on the market becomes bigger than the total deposited margin.
- The funding fee becomes bigger than the total deposited margin.
- A combination of both scenarios above.
Scenario 1 is highly unlikely to happen because insolvent positions would have been liquidated before their debt became bigger than the total deposited margin on the market. Moreover, in a volatile market, the protocol team can adjust the value of
liquidationBufferRatio
to ensure that even sharp price movements don't cause positions to get underwater.Scenario 2 is also highly unlikely to happen because when the funding fee is so big, it's expected that the market will regulate itself, bringing the skew back to zero and decreasing the funding fees. Even if the funding fee starts to rapidly decrease the margins of the positions, liquidators will step up and liquidate those underwater positions before bad debt is accrued.
Given the low probability of this bug happening, I think it deserves medium severity.
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.
I agree with the low probability, but when it happens it definitely hits the High category on both points according to the rules:
- Definite loss of funds without (extensive) limitations of external conditions.
- Inflicts serious non-material losses.
The README states that the protocol is running liquidators, so it's assumed that unhealthy positions are going to be liquidated on time and bad debt is not going to be created:
Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, arbitrage bots, etc.)?
- There are keepers for order execution and liquidations. [...]
I'd say that this itself is an extensive limitation for this issue to happen.
Even though the protocol will be using keepers for order execution and liquidations, it doesn't ensure that every liquidatable position will be liquidated. Despite that fact, the issue will happen in the scenarios listend in the escalation comment here, hence, I believe medium is appropriate here cause it requires specific states, and loss is highly constrained:
Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained.
Planning to accept the escalation and downgrade the issue to Medium.
Result: Medium Unique
The protocol team fixed this issue in the following PRs/commits: https://github.com/dhedge/flatcoin-v1/pull/344
The Lead Senior Watson signed off on the fix.
xiaoming90
high
Code asymmetry of
globalPositions.marginDepositedTotal
Summary
Code asymmetry of
globalPositions.marginDepositedTotal
state variable could lead to an accounting error within the protocol, leading to the protocol being broken. It will also lead to a loss of assets for the LP.Vulnerability Detail
There is a code asymmetry issue in how the
globalPositions.marginDepositedTotal
is calculated within theFlatcoinVault.settleFundingFees
andFlatcoinVault.updateGlobalPositionData
functions.Within the
FlatcoinVault.settleFundingFees
function, theglobalPositions.marginDepositedTotal
is allowed to go to negative. The reason for allowing this is mentioned in the code's commentOnce the underwater position is liquidated, then the funding fees will be reverted and the total will be positive again.
in Line 238 of theFlatcoinVault.settleFundingFees
function. During the liquidation of the underwater position, the liquidation logic will re-adjust the negativeglobalPositions.marginDepositedTotal
to be positive again.https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L224
However, within the
FlatcoinVault.updateGlobalPositionData
functions, it operates differently where theglobalPositions.marginDepositedTotal
cannot become negative. If the value is negative, it will be set to zero, as seen in Line 191 of theFlatcoinVault.updateGlobalPositionData
functions above.https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L175
The misalignment of how the
globalPositions.marginDepositedTotal
should behave within different functions causes various issues within the protocol.Consider the following scenario to demonstrate one of the negative impacts.
stableCollateralTotal
= 25 ETHglobalPositions.marginDepositedTotal
= 10 ETH (Consists of margin of Bob position)Assume the accrued funding fee is 20 ETH, and the long traders must pay the LP (short). Note that these values are intentionally chosen or inflated to demonstrate the accounting/math error in the implementation, and they do not affect the validity of this issue.
Whenever any of the transactions (e.g. announce or execute order, change protocol's parameters) within the protocol is executed, the following
FlatcoinVault.settleFundingFees
function will be executed to settle the global funding fees. In addition, theFlatcoinVault.settleFundingFees
function is also permissionless and can be executed by anyone at any point in time.https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L224
Assume that the
FlatcoinVault.settleFundingFees
function is executed at this point. Line 241 above will be evaluated as follows. TheglobalPositions.marginDepositedTotal
will be updated to -10 ETH.In the comments in Lines 238-240 above within the
FlatcoinVault.settleFundingFees
function, it expects the liquidation process to re-adjust back theglobalPositions.marginDepositedTotal
to positive or the correct value later.Assume that a liquidation is executed. The
vault.updateGlobalPositionData
function at Line 150 within theLiquidationModule.liquidate
function below will be executed, and the value ofmarginDelta
parameter will be as follows:https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/LiquidationModule.sol#L150
Within the
updateGlobalPositionData
function below, Line 183 will evaluate as follows:This is aligned with the earlier code's comments (
Once the underwater position is liquidated, then the funding fees will be reverted and the total will be positive again.
) as theglobalPositions.marginDepositedTotal
indeed is restored back to the positive and correct value after the re-adjustment.https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L175
Unfortunately, in reality, the execution of the
settleFundingFees
function is not always followed by the execution of the liquidation logic. Thus, the logic code expect the liquidation process executed later to always re-adjust theglobalPositions.marginDepositedTotal
value back to the intended value.Let's assume another scenario. Assume that after the
FlatcoinVault.settleFundingFees
function is executed, someone triggers an action (e.g., open, adjust, close order) that executes theupdateGlobalPositionData
function instead of the liquidation. Assume that the_marginDelta
is insignificant in the following:https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L175
When Line 190 above is executed, since
globalPositions.marginDepositedTotal
is negative (-10 ETH), it will be reset to zero. This is an issue because the code earlier assumes that the liquidation process will help to re-adjust the negative value. However, over here, the state has been wiped out.When the liquidation is executed subsequently, the
marginDelta
will be+10 ETH
, similar to the earlier scenario.https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/LiquidationModule.sol#L150
Within the
updateGlobalPositionData
function, theglobalPositions.marginDepositedTotal
will be set to as follows:https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L175
At the end of the liquidation, the long trader side, which had lost all its margin earlier, regained 10 ETH, which is incorrect and shows an error in the protocol's accounting.
The
globalPositions.marginDepositedTotal
is also inflated and not backed by any assets. The system will wrongly assume that there areglobalPositions.marginDepositedTotal
, while in reality, there is none. This results in the system continuing to transfer collateral assets to the LP when settling funding fees when there are none. The LP will receive collateral credit backed by no assets, leading to a loss of assets for them.Impact
The
marginDepositedTotal
state is the key variable in the protocol's account system, and its value will be incorrect, leading to the protocol being broken. It will also lead to a loss of assets for the LP.Code Snippet
https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L224
https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L175
Tool used
Manual Review
Recommendation
Ensure that the behavior of
globalPositions.marginDepositedTotal
(whether it can go negative) is consistent throughout the codebase to prevent any error from occurring.