search

sherlock-audit / 2024-03-flat-money-fix-review-contest-judging

3 stars 2 forks source link

santipu_ - Collateral cap is not correctly checked due to unrealized PnL #7

Closed sherlock-admin3 closed 4 months ago

sherlock-admin3 commented 4 months ago

santipu_

medium

Collateral cap is not correctly checked due to unrealized PnL

Summary

Before LPs deposit funds into the protocol, there is a check to ensure that the total collateral doesn't surpass the cap set by the admins. However, this check doesn't account for unrealized PnL of open leverage positions and that will lead to two different issues.

Vulnerability Detail

When LPs deposit funds into the protocol, they call announceStableDeposit function at DelayedOrder contract. In that function, there is a check that ensures that the total collateral deposited by LPs doesn't surpass the cap set by the admins:

https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/DelayedOrder.sol#L75

vault.checkCollateralCap(depositAmount);

The function checkCollateralCap is the following:

https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L324-L329

    function checkCollateralCap(uint256 _depositAmount) public view {
        uint256 collateralCap = stableCollateralCap;

        if (stableCollateralTotal + _depositAmount > collateralCap)
            revert FlatcoinErrors.DepositCapReached(collateralCap);
    }

However, the variable stableCollateralTotal doesn't include the global profits and losses of the opened positions on the market. Therefore, the value of stableCollateralTotal is not accurate because it doesn't reflect the actual liquidity LPs can access.

This will cause two problematic scenarios:

1. Price of ETH goes up: The profits made by the leverage positions (losses by LPs) won't be accounted into stableCollateralTotal, so when LPs want to deposit more liquidity, the check will fail because stableCollateralTotal will be a higher value than it should.

1. Price of ETH goes down: The losses made by the leverage positions (profits by LPs) won't be accounted into stableCollateralTotal, so LPs will be able to deposit too much liquidity to the market, thereby making the cap meaningless.

Impact

When the price of ETH goes up and some leverage positions are open, LPs won't be able to deposit more liquidity. When the price of ETH goes down and some leverage positions are open, LPs will be able to deposit too much liquidity, making the cap meaningless.

Code Snippet

https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L324-L329

Tool used

Manual Review

Recommendation

Include the global profits and losses into the checkCollateralCap() function.

sherlock-admin2 commented 4 months ago

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

santipu_ commented:

Medium

rashtrakoff commented 4 months ago

The purpose of the stable collateral total cap is to limit the amount deposited. It's by design that we are not accounting for unrealized PnL of leverage traders in determining if the cap has reached. The cap should not be breached due to deposits only.