search

sherlock-audit / 2023-12-flatmoney-judging

11 stars 9 forks source link

alexbabits - User can burn their NFT causing incorrect global position data #85

Closed sherlock-admin closed 8 months ago

sherlock-admin commented 8 months ago

alexbabits

high

User can burn their NFT causing incorrect global position data

Summary

Users can burn their NFT representing their open leverage position.

Alice opens a leverage position, and gets minted an NFT representing that position to her address. She then burns her NFT right before a liquidation or at any other time, at little to no loss for her, since her position was going to get liquidated anyway. (The NFT is only locked and untransferrable during closing/adjusting an open position).

By burning the NFT, this makes the liquidation fail because it cannot burn the already burned NFT. Her _positions mapping for that NFT remains intact and inert as a ghost position in the system. The _positions data for that tokenId of the NFT is perfectly intact but now the underlying NFT is no longer associated with the owner of that position.

The liquidation was the sole way to forcefully delete a NFT position _positions via the vault.deletePosition(tokenId).

(If for some reason user EOA cannot burn their owned NFT, a smart contract can create a position and hold the NFT position, and inherit ERC721LockableEnumerableUpgradeable and will have the ability to call _burn() directly, identically to how the LeverageModule.sol is able to call _burn() directly).

Vulnerability Detail

See Summary.

Impact

The GlobalPositions.marginDepositedTotal is permanently inflated by the burned positions Position.marginDeposited, and the GlobalPositions.sizeOpenedTotal is permanently inflated by Position.additionalSize.

The burned _positions.marginDeposited and _positions.additionalSize get permanently stuck in the system inflating the global values, which negatively impacts the truth of the systems accounting in various places. The skew would be permanently and artificially tilted more towards the leverage longs, which effects the dynamic funding rate, which has an impact on ALL trader/UNIT LP'er accrued funding fee amounts that they have to pay or receive.

This would also disallow the system from completing the final stable coin withdraw, because the check of the sizeOpenedTotal != 0 would get triggered because the stuck long position still exists.

The incorrect VaultSummary trickles down and effects many things related to the global positions. For example, the vault summary effects _getMarketSummaryLongs() which effects fundingAdjustedLonPnLTotal() which effects stableCollateralTotalAfterSettlement() which effects the calculation of the _stableCollateralBalance value, which effects the stableCollateralPerShare() which effects how much UNIT is minted per rETH.

Also, automated keepers would try to liquidate this ghost position whenever it is liquidatable, but their calls would fail (cannot burn already burnt NFT position) and waste gas.

Code Snippet

LeverageModule.executeOpen(), NFT minted to user when opening position: https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L111 LiquidationModule.liquidate() burn of NFT fails because already burnt: https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LiquidationModule.sol#L174 Cannot delete the ghost position through liquidations or any means FlatcoinVault.deletePosition(): https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L157 Cannot withdraw last stable coin request: https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/StableModule.sol#L132-#L135

Tool used

Manual Review

Recommendation

sherlock-admin commented 8 months ago

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

karanctf commented:

qa

takarez commented:

invalid

nevillehuang commented 8 months ago

Invalid, seems to be duplicate of #227, but burning of tokens can only be executed via a liquidation as mentioned by the author. Token is minted via LeverageModule._mint() seen here, so the following seems no possible:

(If for some reason user EOA cannot burn their owned NFT, a smart contract can create a position and hold the NFT position, and inherit ERC721LockableEnumerableUpgradeable and will have the ability to call _burn() directly, identically to how the LeverageModule.sol is able to call _burn() directly).