Position Creation Re-Entrancy Vulnerability

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Position.sol#L148

Vulnerability details

Impact

The Position contract performs a _safeMint operation when actually creating a position via mint. The vulnerability arises from the code's deviation from the Checks-Effects-Interactions (CEI) pattern and in particular, the initId entry written to after creation internally as well as the limitDelay entry written to after creation in the initiateLimitOrder function of Trading. This re-entrancy can be exploited in two ways and as such, this exhibit can be considered as counting for either one or two major vulnerabilities (in the past, some C4 contests have permitted the same flaw exploited in a different manner to be counted twice).

The first vulnerability is significant as it causes the trades function to yield a significantly inflated accInterest variable. This can be exploited by creating a position via initiateMarketOrder in the Trading contract and invoking addToPosition right afterwards. All validation checks are passed and the position now permanently stores the invalid accInterest to storage via the setAccInterest function.

The second vulnerability is also significant and arises from the inexistent prevention of a limit-order duration. Given that limitDelay is never written to, it is possible to immediately create a limit order and consume it in the same transaction via executeLimitOrder further compromising the system by being able to acquire an instant profit.

Proof of Concept

For the first vulnerability, we can observe that the initId entry is written to with a default value causing calculations in the trades function to yield a zero-value in accInterest for a properly initialized NFT. Given that we can re-enter all contracts of the system, we can invoke the addToPosition function with a zero amount to permanently store the inflated accInterest value that is temporarily possible in the hijacked mint call.

All logical checks (_checkOwner, _checkDelay, tradingExtension.validateTrade, _checkVault) will be passed successfully as the entries the mint function of Position writes to after _safeMint are solely utilized during the burn operation and even then remain unvalidated. At this point, we have managed to permanently store the inflated accInterest value which we can claim after the block delay for positions has passed.

For the second vulnerability, we can observe that the limit order delay is written to after the mint operation concludes. This permits us to exploit it, closing our limit order immediately to acquire a profit as well as completely corrupt the storage of Position as it would delete the index of our yet-uncreated position (which is by default 0) in the executeLimitOrder function.

Tools Used

Manual review of the codebase.

Recommended Mitigation Steps

We advise a _mint operation to be utilized instead of a _safeMint operation as a callback operation on the receiver of the NFT is in-fact redundant for the use case of the Tigris protocol. This will completely alleviate both vulnerabilities as they would no longer be possible.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #400

[c4-judge]

GalloDaSballo marked the issue as satisfactory