code-423n4 / 2023-05-particle-findings

0 stars 0 forks source link

Exact `amount` might halt trades #45

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/protocol/ParticleExchange.sol#L331 https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/protocol/ParticleExchange.sol#L428

Vulnerability details

Impact

Some trades cannot be executed where there is slippage involved in the trade.

Proof of Concept

When selling an NFT to the market an exact amount must be specified which must be the exact amount returned by the market place, otherwise it reverts. The market place might only be able to provide an estimate for the amount returned and calculates the exact amount only in the actual transaction. If so the NFT cannot be sold. Likewise, when buying an NFT from the market an exact amount must also be specified which is the exact amount that is paid to the market place and must be the exact decrease in balance at the end of the call. This amount might be excessive, in which case funds are lost, unless the difference is refunded by the market place and sent back, in which case the buying will revert.

Recommended Mitigation Steps

Allow for some slippage tolerance.

Assessed type

DoS

hansfriese commented 1 year ago

I think the marketplace will generally return an accurate estimation and it is likely the protocol's frontend is going to put the two actions into the same block. Will leave it open for the sponsor's review. The point is how the frontend is written.

c4-judge commented 1 year ago

hansfriese marked the issue as satisfactory

c4-sponsor commented 1 year ago

wukong-particle marked the issue as sponsor disputed

wukong-particle commented 1 year ago

Judge is correct. The frontend uses Reservoir and Opensea API to pull the exact amount that will get executed on marketplaces. We don't allow slippage tolerance to prevent leaky buckets.

This is similar to L-3 in https://github.com/code-423n4/2023-05-particle-findings/issues/28.

c4-judge commented 1 year ago

hansfriese marked the issue as unsatisfactory: Invalid

d3e4 commented 1 year ago

Judge is correct. The frontend uses Reservoir and Opensea API to pull the exact amount that will get executed on marketplaces. We don't allow slippage tolerance to prevent leaky buckets.

This is similar to L-3 in #28.

This is indeed a duplicate of L-3 in #28.

Pulling the data from the market and sending the order could be done in the same transaction, but how is the user integrated into all of this? He must initiate an action in the frontend and then approve the transaction. This will generally take longer than one block, so the pulled data that the user approves cannot be from the same block as when he approves the transaction. Not to mention when the transaction is mined. There may also be timed auctions which continously change the amount. Not allowing for slippage requires a perfectly static market for all order types for at least several minutes. What am I missing?

c4-judge commented 1 year ago

hansfriese marked the issue as satisfactory

c4-judge commented 1 year ago

hansfriese changed the severity to QA (Quality Assurance)

wukong-particle commented 1 year ago

We have decided to leave this issue from contract update. The exact amount is pulled from marketplace directly (e.g., Seaport's conduit). The input amount should exactly match what's being traded. If the market fluctuates, the input data should be seen as stale, because it's not up to the protocol to decide what market slippage should be allowed or not. Our frontend and backend system, however, should optimize to reduce real time data discrepancy as much as possible.