c4-bot-9 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/AutoCompound.sol#L159-L172 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1032-L1074

Vulnerability details

The protocol is using block.timestamp as the deadline argument while interacting with the Uniswap NFT Position Manager, which completely defeats the purpose of using a deadline.

Actions in the Uniswap NonfungiblePositionManager contract are protected by a deadline parameter to limit the execution of pending transactions. Functions that modify the liquidity of the pool check this parameter against the current block timestamp in order to discard expired actions.

These interactions with the Uniswap position are present throughout the code base, in particular and not only in the functions V3Utils::_swapAndMint, Automator::_decreaseFullLiquidityAndCollect, LeverageTransformer::leverageUp.. Those functions call their corresponding functions in the Uniswap Position Manager, providing the deadline argument with their own dealine argument.
In the other hand, AutoCompound::execute and V3Vault::_sendPositionValue functions provide block.timestamp as the argument for the deadline parameter in their call to the corresponding underlying Uniswap NonfungiblePositionManager contract.

File: src/transformers/AutoCompound.sol

// deposit liquidity into tokenId
if (state.maxAddAmount0 > 0 || state.maxAddAmount1 > 0) {
    _checkApprovals(state.token0, state.token1);

    (, state.compounded0, state.compounded1) = nonfungiblePositionManager.increaseLiquidity(
        INonfungiblePositionManager.IncreaseLiquidityParams(
@@->    params.tokenId, state.maxAddAmount0, state.maxAddAmount1, 0, 0, block.timestamp
        )
    );

    // fees are always calculated based on added amount (to incentivize optimal swap)
    state.amount0Fees = state.compounded0 * rewardX64 / Q64;
    state.amount1Fees = state.compounded1 * rewardX64 / Q64;
}

File: src/V3Vault.sol

if (liquidity > 0) {
    nonfungiblePositionManager.decreaseLiquidity(
        INonfungiblePositionManager.DecreaseLiquidityParams(tokenId, liquidity, 0, 0, block.timestamp)
    );
}

Using block.timestamp as the deadline is effectively a no-operation that has no effect nor protection. Since block.timestamp will take the timestamp value when the transaction gets mined, the check will end up comparing block.timestamp against the same value (see https://github.com/Uniswap/v3-periphery/blob/697c2474757ea89fec12a4e6db16a574fe259610/contracts/base/PeripheryValidation.sol#L7).

Impact

Failure to provide a proper deadline value enables pending transactions to be maliciously executed at a later point. Transactions that provide an insufficient amount of gas such that they are not mined within a reasonable amount of time, can be picked by malicious actors or MEV bots and executed later in detriment of the submitter.
See this issue for an excellent reference on the topic (the author runs a MEV bot).

Tools Used

Manual review

Recommended Mitigation Steps

As done in the LeverageTransformer::leverageUp and V3Utils::_swapAndIncrease functions, add a deadline parameter to the AutoCompound::execute and V3Vault::_sendPositionValue functions and forward this parameter to the corresponding underlying call to the Uniswap NonfungiblePositionManager contract.