Using `block.timestamp` for swap deadline offers no protection

[hats-bug-reporter[bot]]

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0x4c57953be845adc772b0561d831bd49335ed7d7b134fcf49b248c0dceb1c9bfc Severity: medium

Description: Using block.timestamp for swap deadline offers no protection

Description

In StakingServiceBase.sol::_depositEth:

 function _depositEth(uint256 amountIn, uint256 amountOutMin) internal returns (uint256 amountOut) {
        PoolEthInfo memory _poolEthInfo = poolEthInfo;
        require(_poolEthInfo.poolType != PoolType.DEACTIVATED, "DEPOSIT_ETH_PAUSED");
        if (_poolEthInfo.poolType == PoolType.UNIV2) {
            address[] memory path = new address[](2);
            path[0] = WETH;
            path[1] = _poolEthInfo.token;
            uint256[] memory amounts = UNISWAPV2_ROUTER.swapExactETHForTokens{value: amountIn}(
                amountOutMin,
                path,
                address(this),
                block.timestamp + 1000 //erictee-issue: block.timestamp for swap deadline offers no protection
            );
            amountOut = amounts[1];
        } else if (_poolEthInfo.poolType == PoolType.UNIV3) {
            amountOut = UNISWAPV3_ROUTER.exactInputSingle{value: amountIn}(
                IUniv3Router.ExactInputSingleParams({
                    tokenIn: WETH,
                    tokenOut: _poolEthInfo.token,
                    fee: _poolEthInfo.fee,
                    recipient: address(this),
                    deadline: block.timestamp + 1000, //erictee-issue: block.timestamp for swap deadline offers no protection
                    amountIn: amountIn,
                    amountOutMinimum: amountOutMin,
                    sqrtPriceLimitX96: 0
                })
            );
        // REDACTED by ERICTEE

Noticed that the swap deadline is set to block.timestamp + 1000 for both UNISWAPV2_ROUTER and UNISWAPV3_ROUTER.

Attack Scenario

In the PoS model, proposers know well in advance if they will propose one or consecutive blocks ahead of time. In such a scenario, a malicious validator can hold back the transaction and execute it at a more favourable block number.

This offers no protection as block.timestamp will have the value of whichever block the txn is inserted into, hence the txn can be held indefinitely by malicious validators.

Attachments

NA

1. Proof of Concept (PoC) File

NA

2. Revised Code File (Optional)

Consider allowing function caller to specify swap deadline input parameter.

[PlamenTSV]

The deployment will be mainnet only, so there is no risk of block-stuffing or griefing, but it is correct using block.timestamp is incorrect since it disables the deadline. Deadline should be an input parameter. I believe it is LOW, leaving for sponsor comment if needed.

[0xEricTee]

Similar issues reported in other platforms which were awarded as High/Medium severity:

1) https://code4rena.com/reports/2023-08-pooltogether#m-03--missing-deadline-param-in-swapexactamountout-allowing-outdated-slippage-and-allow-pending-transaction-to-be-executed-unexpectedly

2) https://code4rena.com/reports/2023-08-dopex#m-19-using-blocktimestamp-as-the-deadlineexpiry-invites-mev

3) https://github.com/sherlock-audit/2023-04-blueberry-judging/issues/145

[PlamenTSV]

The 2 points I see are:

1. Assumption on validator action which can be considered centralizatio/network risk. Keep in mind that on the mainnet block-stuffing is very high cost/impossible.
2. The check really is not done correctly and even though the provided issues are fairly outdated and more recent ones (some of them personal) have even been invalidated, I can see the impact.

I will give this the medium label aswell but keep the low and let the sponsor decide how severe and likely this is, since this also depends WETH/CVX pool and it's liquidity. I hope this is fine by you.

[kakarottosama]

quite understand why Plamen intended to set this a low instead of medium. Agree on reasoning to perform the 'attack' is quite tricky as one need to be selected validator. Not really sure if this kind attack is commonly exist in nature, or it's just a theoretical one and not practicable.

The reasoning of no protection is also debatable since there is amountOutMin which protect the output result. Sometimes past audit report might be obsolete in time after some further analysis, which is quite common these days.

[PlamenTSV]

quite understand why Plamen intended to set this a low instead of medium. Agree on reasoning to perform the 'attack' is quite tricky as one need to be selected validator. Not really sure if this kind attack is commonly exist in nature, or it's just a theoretical one and not practicable.

The reasoning of no protection is also debatable since there is amountOutMin which protect the output result. Sometimes past audit report might be obsolete in time after some further analysis, which is quite common these days.

Completely agree. With the points listed above, I believe the sponsor can decide which severity is more appropriate.

[rodiontr]

@PlamenTSV Hey, this is invalid, in the function block.timestamp is used but the issue says block.number

[0xMR0]

@PlamenTSV i judge audit contest at sherlock. i believe the issue is not valid. The auditor has mentioned in report the txn can be held indefinitely by malicious validators. This is not correct, as the deadline is correctly hardcoded and it seems to be intended design by project team because the transaction will expire after 1000 seconds i.e 16.66 minutes (deadline is set as deadline: block.timestamp + 1000, . The above reference by auditor describes different scenarios and those are not relevant to this issue. At best, this is informational issue

[PlamenTSV]

block.timestamp takes the value of the timestamp at which the tx gets executed, not queued for execution. So whenever the transaction gets executed, the deadline protection will pass. The assumption is "When a user sends the transaction, it will fail if 1000 seconds pass" The reality is "When a user sends a transaction, it can be executed whenever in the future and when it gets executed, then we add 1000 to it's current timestamp, not the timestamp when it was sent" This is the entire issue. I believe it is low since validators are trusted entities and they will get slashed extremely hard and would get no reward from withholding blocks. I kept medium due to historical decisions and let the sponsor decide but I stand that it should be LOW

[0xMR0]

so you mean amountOutMinimum: amountOutMin, slippage protection is of no use? In either case, the user would get minimum specified tokens?

[rodiontr]

so you mean amountOutMinimum: amountOutMin, slippage protection is of no use? In either case, the user would get minimum specified tokens?

Check the issue again, it says “block.number” but the deadline is block.timestamp. Of course it’s invalid

[shalbe-cvg]

Hello,

Thank you for taking your time to report this issue. However, we do consider that having a slippage (and therefore a minimum amount to receive with a swap) is enough protection for the user. Furthermore, this is unlikely to happen on Ethereum mainnet since validators are trusted entities.

Regards