chaduke - PerpMath._profitLoss() caluclates the profit/loss wrongly.

[sherlock-admin2]

chaduke

high

PerpMath._profitLoss() caluclates the profit/loss wrongly.

Summary

PerpMath._profitLoss() caluclates the profit/loss wrongly. The main problem is that is uses price instead of lastPrice as the denominator. As a result, profit is calculated larger than expected and loss is calculated smaller than exepcted.

Vulnerability Detail

1. The PerpMath._profitLoss() calculates the profit and loss based on the new price and the lastPrice. However, it uses price instead of lastPrice as the denominator. As a result, profit is calculated larger than expected and loss is calculated smaller than exepcted.

https://github.com/sherlock-audit/2023-12-flatmoney/blob/bba4f077a64f43fbd565f8983388d0e985cb85db/flatcoin-v1/src/libraries/PerpMath.sol#L175-L184

2. For example, suppose lastPrice = 100, additionalSize = 1000, and new price = 120, so priceShift = 20. the profit should be 20*1000/100 = 200. However, using the implementation, the calculated profit is only 165. On the otherhand, when the price decreases, the donominator will use the new price which is smaller than it is supposed to be. As a result, the loss will be enlarged by mistake.

The following POC shows my finding:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";

contract PerpMath {
    uint256  public lastPrice = 100;
    uint256 additionalSize = 1000;

    // new price = 120, so I should gain 1000*20/100 = 200

    function _profitLoss(uint256 price) public view returns (int256 pnl) {
        int256 priceShift = int256(price) - int256(lastPrice);
        int256 profitLossTimesTen = int256(additionalSize) * (priceShift) * 10 / int256(price);

        if (profitLossTimesTen % 10 != 0) {
            return profitLossTimesTen / 10 - 1;
        } else {
            return profitLossTimesTen / 10;
        }
    }

}

contract FundingTest is Test {
    PerpMath p;

    function setUp() public {
        p = new PerpMath();
    }

    function testPnl() public{
        int256 pnl = p._profitLoss(120);

        console2.log("pnl: %", pnl);
    }
}

Impact

PerpMath._profitLoss() caluclates the profit/loss wrongly. The main problem is that is uses price instead of lastPrice as the denominator. As a result, profit is calculated larger than expected and loss is calculated smaller than exepcted.

Code Snippet

Tool used

https://github.com/sherlock-audit/2023-12-flatmoney/blob/bba4f077a64f43fbd565f8983388d0e985cb85db/flatcoin-v1/src/libraries/PerpMath.sol#L175-L184

Manual Review

Recommendation

[sherlock-admin]

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

takarez commented:

invalid

[ydspa]

Escalate

This finding is invalid, it's not a dup of the primary issue. This finding says lastPrice should be denominator, but actually using currentPrice as denominator is correct.

Let's use the case in this finding for illustration

 lastPrice = 100, additionalSize = 1000, and new price = 120
=> profitInUsd = 1000 rETH * ($120 - $100) = $20,000

// the current implementation, match the intended profit as $20,000
profitInRETH = profitInUsd / currentPrice = $20,000 / $120 = 166.6 rETH
profitInUsd = profitInRETH  * currentRETHPrice  = 166.6 rETH * $120 = $20,000

// the recommended solution by this finding, not match 
profitInRETH = profitInUsd / lastPrice = $20,000 / $100 = 200 rETH
profitInUsd = profitInRETH  * currentRETHPrice = 200 rETH * $120 = $24,000

We can see the current implementation, using currentPrice as denominator, is correct. And the recommended solution is wrong.

All in all, the root cause of the primary issue, as described in another dup(https://github.com/sherlock-audit/2023-12-flatmoney-judging/issues/161), is settling PnL before close or liquidation of position, rather than using currentPrice as denominator.

[sherlock-admin2]

Escalate

This finding is invalid, it's not a dup of the primary issue. This finding says lastPrice should be denominator, but actually using currentPrice as denominator is correct.

Let's use the case in this finding for illustration

 lastPrice = 100, additionalSize = 1000, and new price = 120
=> profitInUsd = 1000 rETH * ($120 - $100) = $20,000

// the current implementation, match the intended profit as $20,000
profitInRETH = profitInUsd / currentPrice = $20,000 / $120 = 166.6 rETH
profitInUsd = profitInRETH  * currentRETHPrice  = 166.6 rETH * $120 = $20,000

// the recommended solution by this finding, not match 
profitInRETH = profitInUsd / lastPrice = $20,000 / $100 = 200 rETH
profitInUsd = profitInRETH  * currentRETHPrice = 200 rETH * $120 = $24,000

We can see the current implementation, using currentPrice as denominator, is correct. And the recommended solution is wrong.

All in all, the root cause of the primary issue, as described in another dup(https://github.com/sherlock-audit/2023-12-flatmoney-judging/issues/161), is settling PnL before close or liquidation of position, rather than using currentPrice as denominator.

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.

[nevillehuang]

Agree with @ydspa, this issue should be invalid.

[Evert0x]

Agree with the escalation, the report fails to describe the core issue.

Planning to accept the escalation and remove duplication status

[Czar102]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ydspa: accepted