Functions of Trading contract can be reentered by Position.sol#mint

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Position.sol#L131-L161

Vulnerability details

Impact

Both the contracts of Position and Trading may not work correctly.

Proof of Concept

The Position.sol#mint calls _safeMint will trigger a _checkOnERC721Received callback, which can be used to reenter.

Crackers can use this vulnerability to attack the protocol.

For example, the cracker could send a reentrant attack tx with a call stack like this:

1. cracker calls his own contract Cx.
2. in Cx: calls Trading.sol#initiateMarketOrder
3. in Trading: calls Position.sol#mint
4. in Position: calls Cx#_checkOnERC721Received
5. in Cx: calls Trading.sol#liquidatePosition (reenter here)
6. in Trading: calls Position.sol#burn
7. in Cx: calls Cx#initiateLimitOrder
8. in Trading: calls Position.sol#mint
9. in Position: calls Cx#_checkOnERC721Received

In step 6, it burned the Position NFT that created in step 3, and the state will be updated in a confusing way because the burnt NFT hasn't done state initialization in step 3 yet. In step 8, it created another Position NFT with the same tokenId as the one in step 3, which is also an anomaly. After step 9, when the stack returns to step 3, the burnt NFT will now go on the state initialization.

Tools Used

Manual

Recommended Mitigation Steps

Simplest solution: replace _safeMint with _mint.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #400

[c4-judge]

GalloDaSballo marked the issue as satisfactory