Interactions with AMMs do not use deadlines for operations

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/NTokenUniswapV3.sol#L51-L76

Vulnerability details

Impact

From a judge's contest in a previous contest:

Because Front-running is a key aspect of AMM design, deadline is a useful tool to ensure that your tx cannot be “saved for later”.

Due to the removal of the check, it may be more profitable for a miner to deny the transaction from being mined until the transaction incurs the maximum amount of slippage.

Most of the functions that interact with AMM pools do not have a deadline parameter, but specifically the one shown below is passing block.timestamp to a pool, which means that whenever the miner decides to include the txn in a block, it will be valid at that time, since block.timestamp will be the current timestamp.

Proof of Concept

File: /paraspace-core/contracts/protocol/tokenization/NTokenUniswapV3.sol   #1

51       function _decreaseLiquidity(
52           address user,
53           uint256 tokenId,
54           uint128 liquidityDecrease,
55           uint256 amount0Min,
56           uint256 amount1Min,
57           bool receiveEthAsWeth
58       ) internal returns (uint256 amount0, uint256 amount1) {
59           if (liquidityDecrease > 0) {
60               // amount0Min and amount1Min are price slippage checks
61               // if the amount received after burning is not greater than these minimums, transaction will fail
62               INonfungiblePositionManager.DecreaseLiquidityParams
63                   memory params = INonfungiblePositionManager
64                       .DecreaseLiquidityParams({
65                           tokenId: tokenId,
66                           liquidity: liquidityDecrease,
67                           amount0Min: amount0Min,
68                           amount1Min: amount1Min,
69 @>                        deadline: block.timestamp
70                       });
71   
72               INonfungiblePositionManager(_underlyingAsset).decreaseLiquidity(
73                   params
74               );
75           }
76:  

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/NTokenUniswapV3.sol#L51-L76

A malicious miner can hold the transaction, which may be being done in order to free up capital to ensure that there are funds available to do operations to prevent a liquidation. It is highly likely that a liquidation is more profitable for a miner to mine, with its associated follow-on transactions, than to allow the decrease of liquidity. A miner can also just hold it until maximum slippage is incurred, as the judge stated.

Tools Used

Code inspection

Recommended Mitigation Steps

Add deadline arguments to all functions that interact with AMMs, and pass it along to AMM calls

[c4-judge]

dmvt marked the issue as primary issue

[c4-judge]

dmvt marked the issue as selected for report

[trust1995]

From a practical point of view, this would require the collusion of a large amount of miners which is extraordinarily unlikely. Each miner is incentivized to maximize TX fees in a block, rather than plan a theoretical liquidation some long time in the future.
decreaseLiquidity() properly sends slippage thresholds. For these reasons, I would view this as a useful suggestion rather than M-level (assets are at theoretical risk).

[vladbochok]

@trust1995 I didn't even participate in the contest, so I don't have any incentives. However, I see it very unfair to judge it as "suggestion".

The described report logically fully satisfies the description of the Medium severity bug. From the medium severity bug description.

Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Doesn't the report show hypothetical attack paths with external requirements?

[trust1995]

I don't see this attack as different from general miner collusion, i.e. when deadline parameters are passed by user "properly". It is a property of Eth style blockchains.
The additional incentive presented for miner collusion is theoretical. Also, the funds in the NTokenUniswap are counted as collateral, so this action doesnt save user from a liquidation in any reasonable way.

[dmvt]

To invalidate this or mark it as a suggestion is the same as asserting that the deadline timestamp is an unnecessary part of an AMM design. The recent decision linked in the issue states exactly the opposite.

I agree that:

Because Front-running is a key aspect of AMM design, deadline is a useful tool to ensure that your tx cannot be “saved for later”.

I also agree with the medium risk rating given to the issue in the previous contest. Those these two issues do diverge slightly, the risk and impact are the same.

[trust1995]

Contrarily, to mark the decision as M means issue has demonstrated a core dis-functioning, or a theoretical risk of funds with stated hypotheticals. Neither is evident from the issue.

A malicious miner can hold the transaction, which may be being done in order to free up capital to ensure that there are funds available to do operations to prevent a liquidation. It is highly likely that a liquidation is more profitable for a miner to mine, with its associated follow-on transactions, than to allow the decrease of liquidity. A miner can also just hold it until maximum slippage is incurred, as the judge stated.

There is no actual risk of funds here. The NToken holding counts as collateral.

I agree deadline is a useful tool for AMMs. I disagree that the way in which it is used here equates to a M-level impact.

[dmvt]

I agree deadline is a useful tool for AMMs. I disagree that the way in which it is used here equates to a M-level impact.

Your opinion is noted, but in the absence of further information, this issue is valid and will remain medium risk.