Nftperp Exchange audit
A private security review of NftPerp (NFT perpetuals), conducted with Three Sigma and completed on 2nd January 2024 over 25 days. 0xSimao disclosed 20 findings: 2 high severity, 5 medium, 4 low and 9 informational.
What NftPerp is
Nftperp lets traders take leveraged long and short positions on NFT collection floor prices, Milady, Pudgy Penguins and the rest, through perpetual futures settled entirely in ETH against a virtual AMM.
Scope
This first round covered the exchange core: margin accounting as positions are reduced or merged, funding settlement, notional bookkeeping, and the maker/taker fill machinery of the order flow.
High severity findings
- reduceOnly limit order update does not take into account that a new amm may be selected, which may lead to loss of funds
- Realized pnl not returned to trader on _mergeDecrease()
Medium severity findings
- _getPositionNotional() crops the size of the position to the available reserves, which may lead to unexpected profits/losses
- Partially removing liquidity may underflow if the price of the pool has changed significantly
- removeLiquidity() in the amm calculates marginToRemove without updating the margin with the funding payment
- _getPriceToTick() reverts if the price is smaller than 1e10, which may happen in _search() as it assumes the final tick as going entirely to the amm
- setFundingPeriod() can be DoSed due to calling settleFunding()
Low severity findings
- In fillPair(), when the taker is the maker, the reduceOnly order mapping is not being deleted
- getTriggerOrders() should be paginated or it may revert if enough orders are created
- PriceFeed: Two step owner transfers are safer
- Total Position Size isn't updated properly
Informational findings
- State changes should always emit events
- Swapping amounts that would send the price below/above the bounds of the amm underflows without a reason
- PriceFeed: First owner isn't set as valid keeper
- Structs can be packed to save gas
- Contracts should inherit their interfaces
- Bug in _reversePosition() might lead to future exploits
- Using low level pop is not recommended
- Errors could include relevant arguments whenever possible, making it easier to debug
- mul() and div() in NFTPMath are misleading due to scaling by 1e18 underneath
The report