c4-bot-8 commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/tools/ZetaTokenConsumerUniV3.strategy.sol#L111 https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/tools/ZetaTokenConsumerUniV3.strategy.sol#L136 https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/tools/ZetaTokenConsumerUniV3.strategy.sol#L171 https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/tools/ZetaTokenConsumerUniV2.strategy.sol#L87 https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/tools/ZetaTokenConsumerUniV2.strategy.sol#L115 https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/tools/ZetaTokenConsumerUniV2.strategy.sol#L153

Vulnerability details

Impact

Loss of fund due to sandwich attack at maximum slippage in turbulent market condition.

Proof of Concept

The main issue is that the deadline is set with a dynamic value block.timestamp. Let's remove the MAX_DEADLINE added to the block.timestamp for simplicity for now.

The block.timestamp will always be the block.timestamp at any time of execution. The block.timestamp is not a fixed unix time so calculate deadline offchain and pass it as a parameter.

To calculate the deadline, get the current block.timestamp offchain which is 1702501995 at the time of writting this report then add how many seconds more you want the transaction to be valid from now. Let's say 10 mins more before the transaction reverts. Then add 10mins to the current block.timestamp then pass the literal value as input.

1702501395 + (10 * 60) = 1702501995.

So pass 1702501995 as deadline.

The main takeaway here is that 1702501995 passed to the swap function as deadline is a fixed unix time in future(10mins) but block.timestamp is dynamic that depends on the time of execution.

Below is the modifier used to check if the deadline has passed.

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

When you pass block.timestamp as the input above modifier then you will be checking the block.timestamp against block.timestamp any time the transaction is executed making the deadline check useless.

//The check becomes this.
require(block.timestamp <= block.timestamp, 'Transaction too old');

This allows miners to "save the tx for later" when there will be maximum slippage and MEV gains through frontrunning bots.

The Uniswap docs defines deadline as "the unix time after which a transaction will be reverted, to protect against long delays and the increased chance of large price swings therein" ref: https://docs.uniswap.org/contracts/v3/guides/swaps/single-swaps#swap-input-parameters

So deadline should be passed as input just like the minimum output.

Tools Used

Uniswap checkdeadline modifier https://github.com/Uniswap/v3-periphery/blob/697c2474757ea89fec12a4e6db16a574fe259610/contracts/base/PeripheryValidation.sol

Recommended Mitigation Steps

The deadline literal should be gotten offchain and passed as input parameter. For example the current block timestamp at the time of this report is 1702501395.

So if a user wants the transaction to be invalid after 10mins. Convert 10mins to seconds and add to the current timestamp. 1702501395 + (10 * 60) = 1702501995.

The deadline that should be passed to the function is 1702501995.

1702501995 is a fixed literal and would make the transaction revert when the block.timestamp exceeds this fixed timestamp literal.

Assessed type

MEV

c4-pre-sort commented 11 months ago

DadeKuma marked the issue as duplicate of #122

c4-pre-sort commented 11 months ago

DadeKuma marked the issue as insufficient quality report

c4-judge commented 10 months ago

0xean changed the severity to QA (Quality Assurance)