Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Deadline check for UniswapAdapter is ineffective

elolpuer

Summary

Protocol uses Uniswap V2 and Uniswap V3 adapters for few functions: CreditDelegationBranch.sol:convertMarketsCreditDepositsToUsdc(), CreditDelegationBranch.sol:rebalanceVaultsAssets(), CreditDelegationBranch.sol:settleVaultsDebt() and FeeDistributionBranch.sol:convertAccumulatedFeesToWeth(). All these functions are executed by Chainlink Keepers (not the contract owner) and depend on asset prices, making it crucial to use the deadline parameter correctly to avoid outdated slippage.
However, the current implementation sets the deadline parameter only by the owner, which can be ineffective, because owner can't be 100% sure when performUpkeep functions will be called. Instead, the deadline should be dynamically set based on the timestamp of the performUpkeep function call in Chainlink Keepers to ensure timely execution of swaps.

Vulnerability Details

deadline parameter is set by the owner and is used universally in Uniswap adapters regardless of the swap call time:
BaseAdapter.sol:setDeadline():

/// @notice Sets deadline
/// @param _deadline The new deadline
function setDeadline(uint256 _deadline) public onlyOwner {
// revert if the deadline is in the past
if (_deadline < block.timestamp) revert Errors.SwapDeadlineInThePast();
​
// set the new deadline
deadline = _deadline;
​
// emit the event
emit LogSetDeadline(_deadline);
}

UniswapV2Adapter.sol:

/// @inheritdoc IDexAdapter
function executeSwapExactInputSingle(SwapExactInputSinglePayload calldata swapPayload)
external
returns (uint256 amountOut)
{
...
uint256[] memory amountsOut = IUniswapV2Router02(uniswapV2SwapStrategyRouterCache).swapExactTokensForTokens({
amountIn: swapPayload.amountIn,
amountOutMin: amountOutMinimum,
path: path,
to: swapPayload.recipient,
deadline: deadline //@audit deadline is not dynamic
});
​
return amountsOut[1];
}
​
/// @inheritdoc IDexAdapter
function executeSwapExactInput(SwapExactInputPayload calldata swapPayload) external returns (uint256 amountOut) {
...
// execute trade
uint256[] memory amountsOut = IUniswapV2Router02(uniswapV2SwapStrategyRouterCache).swapExactTokensForTokens({
amountIn: swapPayload.amountIn,
amountOutMin: amountOutMinimum,
path: tokens,
to: swapPayload.recipient,
deadline: deadline //@audit deadline is not dynamic
});
​
// return the amount out of the last trade
return amountsOut[tokens.length - 1];
}

UniswapV3Adapter.sol:

/// inheritdoc IDexAdapter
function executeSwapExactInputSingle(SwapExactInputSinglePayload calldata swapPayload)
external
returns (uint256 amountOut)
{
...
return swapRouter.exactInputSingle(
IUniswapV3RouterInterface.ExactInputSingleParams({
tokenIn: swapPayload.tokenIn,
tokenOut: swapPayload.tokenOut,
fee: feeBps,
recipient: swapPayload.recipient,
deadline: deadline,//@audit deadline is not dynamic
amountIn: swapPayload.amountIn,
amountOutMinimum: amountOutMin,
sqrtPriceLimitX96: 0
})
);
}
​
/// inheritdoc IDexAdapter
function executeSwapExactInput(SwapExactInputPayload calldata swapPayload) external returns (uint256 amountOut) {
...
return swapRouter.exactInput(
IUniswapV3RouterInterface.ExactInputParams({
path: swapPayload.path,
recipient: swapPayload.recipient,
deadline: deadline,//@audit deadline is not dynamic
amountIn: swapPayload.amountIn,
amountOutMinimum: amountOutMinimum
})
);
}

PoC

Take for example FeeConversionKeeper (for PoC just "keeper") and assume that owner don't know exactly when performUpkeep will be called, so he sets the deadline = block.timestamp + 1 days this can lead to unintended consequences.

1 - Keeper calls checkUpkeep() and checks if fee distribution is needed for asset:

function checkUpkeep(bytes calldata /**/ ) external view returns (bool upkeepNeeded, bytes memory performData) {
...
// Iterate over markets by id
for (uint128 i; i < liveMarketIds.length; i++) {
...
// Iterate over receivedMarketFees
for (uint128 j; j < marketAssets.length; j++) {
//@audit check if distribution needed
distributionNeeded = checkFeeDistributionNeeded(marketAssets[j], feesCollected[j]);
​
if (distributionNeeded) {
// set upkeepNeeded = true
upkeepNeeded = true;
...
}
}
}
...
}
​
function checkFeeDistributionNeeded(
address asset,
uint256 collectedFee
)
public
view
returns (bool distributionNeeded)
{
// load keeper data from storage
FeeConversionKeeperStorage storage self = _getFeeConversionKeeperStorage();
​
/// get asset value in USD
//@audit get value depending on the price right now
uint256 assetValue = self.marketMakingEngine.getAssetValue(asset, collectedFee);
​
// if asset value GT min distribution value return true
distributionNeeded = assetValue > self.minFeeDistributionValueUsd;
}

2 - Now if distributionNeeded keeper calls performUpkeep() with marketMakingEngine.convertAccumulatedFeesToWeth in which contract swaps assets for WETH with deadline approximatly block.timestamp + 1 days:

function performUpkeep(bytes calldata performData) external override onlyForwarder {
FeeConversionKeeperStorage memory self = _getFeeConversionKeeperStorage();
​
IMarketMakingEngine marketMakingEngine = self.marketMakingEngine;
​
// decode performData
(uint128[] memory marketIds, address[] memory assets) = abi.decode(performData, (uint128[], address[]));
​
// convert accumulated fees to weth for decoded markets and assets
for (uint256 i; i < marketIds.length; i++) {
marketMakingEngine.convertAccumulatedFeesToWeth(marketIds[i], assets[i], self.dexSwapStrategyId, "");
}
}

3 - Due to high network load or low gas fees transaction is stuck and does not being included in the block right away.

4 - Now price of the asset drops and distributionNeeded = assetValue > self.minFeeDistributionValueUsd = false. (I know that contract sets amountMin with current slippageToleranceBps which can be minimum = 1%, but still followed condition can be false and current slippageToleranceBps can be actually set more than 1% by owner)

5 - But because deadline for swap is not dynamic depending on the swap call - asset will be swaped with lower price later, instead of being reverted.

Impact

Trading activity is very time senstive and failing to set an appropriate deadline allows pending transactions to be executed later, even if the original conditions for the call have changed, potentially leading to unintended losses.

Tools Used

Manual Review

Recommendations

Set the deadline parameter for each swap separately depending on when the function was called, something like:
block.timestamp + minimumDeadline

Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.