c4-bot-1 commented 6 months ago

Lines of code

Vulnerability details

C4 issue

M-21: Dangerous use of deadline parameter

Comments

Revert makes several calls to Uniswap V3. One of Uniswap's security features is the deadline parameter, which sets a time deadline for when a transaction is valid. Unfortunately, in various calls Revert passes in the block.timestamp as the deadline parameter. This makes the check null-and-void as Uniswap compares the value Revert passes it to the block.timestamp as well:

// https://github.com/Uniswap/v3-periphery/blob/697c2474757ea89fec12a4e6db16a574fe259610/contracts/base/PeripheryValidation.sol#L6-L11

modifier checkDeadline(uint256 deadline) {
    require(_blockTimestamp() <= deadline, 'Transaction too old');
    _;
}

Revert incorrectly implements this deadline feature in both V3Vault._sendPositionValue() and AutoCompound.execute() by utilizing the block.timestamp as the deadline.

Mitigation

PR #24

To resolve this issue, Revert instead of using block.timestamp now utilizes user input as the deadline. For V3Vault._sendPositionValue(), Revert modifies the _sendPositionValue() to accept a deadline argument which is used in NonfungiblePositionManager.increaseLiquidity(). Note that this change requires:

modifying FlashLoanLiquidator.liquidate() to allow flash loan liquidators to pass in a deadline, which is used in _sendPositionValue().
modifying V3Vault.liquidate() to allow liquidators to pass in a deadline, which is used in _sendPositionValue().

For AutoCompound::execute(), the same fix is applied where an additional argument is passed into AutoCompound::execute() which represents a deadline defined by the user. This argument is correctly passed into NonfungiblePositionManager.increaseLiquidity().

Anything Else We Should Know

I also grepped for all cases of deadline usage and confirmed that user input now defines the deadline.

Conclusion

LGTM

c4-judge commented 6 months ago

jhsagd76 marked the issue as satisfactory