Tigris Trade contest - 0xsomeone's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Lock.sol#L78-L92

Vulnerability details

Impact

The Lock contract is meant to be the manager of the BondNFT implementation and performs accounting of asset inflow and outflow via the totalLocked variable that is incremented during a lock and decremented during a release. The issue arises in the fact that the extendLock function can increase the assets of a particular bond but does not increment the totalLocked variable.

As a result, when the release function is executed the totalLocked variable will be decremented for more than it has accounted for thus causing whatever funds were deposited using extendLock to be un-retrievable either for the user that deposited them or, in a worst case malicious attack, for the remaining users that have yet not withdrawn their locks.

Proof of Concept

We can create a 7-day empty bond and then invoke extendLock with a significant amount of funds, specifying the minimum period possible.

If no one detects our significantly large deposit for 7 days and withdraws, we can perform a withdrawal at the end and all funds we have deposited will "tap" into the totalLocked allocation of other deposits. This will cause the other deposits to no longer be withdrawable due to an underflow that would occur in release.

Given that you cannot release a bond early, the attack is trivial to execute if users specify a period larger than 7 days for their deposit (which they are presumably incentivized to do so under the Tigris ecosystem).

Tools Used

Manual review.

Recommended Mitigation Steps

We advise the totalLocked variable to be properly incremented during an extendLock operation as well. Given that the variable entry is purely utilized for off-chain purposes, it could potentially be omitted entirely as well since other measurement methodologies can be used such as actual EIP-20 asset balances.

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.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L39-L51 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L60-L72

Vulnerability details

Impact

The StableVault contract attempts to group all types of stablecoins under a single token which can be minted for any of the stablecoins supported by the system as well as burned for any of them.

This is at minimum a medium-severity vulnerability as the balance sheet of the StableVault will consist of multiple assets which do not have a one-to-one exchange ratio between them as can be observed by trading pools such as Curve as well as the Chainlink oracle reported prices themselves.

Given that the contract exposes a 0% slippage 1-to-1 exchange between assets that in reality have varying prices, the balance sheet of the contract can be arbitraged (especially by flash-loans) to swap an undesirable asset (i.e. USDC which at the time of submission was valued at 0.99994853 USD) for a more desirable asset (i.e. USDT which at the time of submission was valued at 1.00000000 USD) acquiring an arbitrage in the price by selling the traded asset.

Proof of Concept

To illustrate the issue, simply view the exchange output you would get for swapping your USDC to USDT in a stablecoin pool (i.e. CurveFi) and then proceed to invoke deposit with your USDC asset and retrieve your incorrectly calculated USDT equivalent via withdraw.

The arbitrage can be observed by assessing the difference in the trade outputs and can be capitalized by selling our newly acquired USDT for USDC on the stablecoin pair we assessed earlier, ultimately ending up with a greater amount of USDC than we started with. This type of attack can be extrapolated by utilizing a flash-loan rather than our personal funds.

Tools Used

Manual review of the codebase, Chainlink oracle resources, Curve Finance pools.

Recommended Mitigation Steps

We advise the StableVault to utilize Chainlink oracles for evaluating the inflow of assets instead, ensuring that all inflows and outflows of stablecoins are fairly evaluated based on their "neutral" USD price rather than their subjective on-chain price or equality assumption.