search

sherlock-audit / 2024-05-elfi-protocol-judging

0 stars 0 forks source link

mstpr-brainbot - The `autoReducePositions` function is not updating the market funding and pool borrowing fees prior to closing the position #52

Open sherlock-admin2 opened 2 weeks ago

sherlock-admin2 commented 2 weeks ago

mstpr-brainbot

High

The autoReducePositions function is not updating the market funding and pool borrowing fees prior to closing the position

Summary

When positions are decreased via autoReducePositions call, borrowing and funding fees are not updated which leads to inaccurate fee accounting for the users.

Vulnerability Detail

The markets overall borrowing and funding rates are updated in the casual flow of decrease position via these function calls:

function _executeDecreaseOrder(
        uint256 orderId,
        Order.OrderInfo memory order,
        Symbol.Props memory symbolProps
    ) internal {
        .
        .
        -> MarketProcess.updateMarketFundingFeeRate(symbolProps.code);
       ->  MarketProcess.updatePoolBorrowingFeeRate(symbolProps.stakeToken, position.isLong, order.marginToken);
}

This update is very crucial because it will accrue the interest up to the current time for both funding and borrowing fees, which are essential for determining users' and pools' profit/loss. However, as we can observe, the autoReducePositions function skips updating the rates and directly closes the position.

Impact

Latest accrued interest will not be updated for the user and previous values will be used. User can get a higher profit or a lesser loss or completely opposites of these. This will also break the entire Masterchef alike flywheel and affect other users as well. Hence, high.

Code Snippet

https://github.com/sherlock-audit/2024-05-elfi-protocol/blob/8a1a01804a7de7f73a04d794bf6b8104528681ad/elfi-perp-contracts/contracts/facets/PositionFacet.sol#L196-L216 https://github.com/sherlock-audit/2024-05-elfi-protocol/blob/8a1a01804a7de7f73a04d794bf6b8104528681ad/elfi-perp-contracts/contracts/process/OrderProcess.sol#L414-L451

Tool used

Manual Review

Recommendation

Add these two functions before calling the autoReducePositions:

MarketProcess.updateMarketFundingFeeRate(symbolProps.code);
MarketProcess.updatePoolBorrowingFeeRate(symbolProps.stakeToken, position.isLong, order.marginToken);
sherlock-admin2 commented 1 week ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/0xCedar/elfi-perp-contracts/pull/56

mstpr commented 2 days ago

Escalate

This should be marked as high. Closing a position without accruing the funding and borrowing fees will result in:

This is also at the same level as issue #63, so there's no reason to keep this marked as medium.

sherlock-admin3 commented 2 days ago

Escalate

This should be marked as high. Closing a position without accruing the funding and borrowing fees will result in:

  • Unfair profit/loss for the trader
  • Unfair borrowing fees for the Lp'ers
  • Depending on how late it's called, it can even create insolvency

This is also at the same level as issue #63, so there's no reason to keep this marked as medium.

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 commented 2 days ago

@0xELFi @0xELFi02 How often would autoReducePositions be called to reduce risk? Trying to assess likelihood here. If often I believe this can be high, but would be a duplicate of #63, given they are pointing to the same root cause of lacking updates of borrowing fees but in different functionalities.