The deposit - withdraw - trade transaction lack of expiration timestamp check

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L877-L917
https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L920-L952

Vulnerability details

The protocol missing the DEADLINE check at all in logic.
This is actually how uniswap implemented the deadline
https://github.com/Uniswap/v2-periphery/blob/0335e8f7e1bd1e8d8329fd300aea2ef2f36dd19f/contracts/UniswapV2Router02.sol#L32-L76

// **** ADD LIQUIDITY ****
function _addLiquidity(
    address tokenA,
    address tokenB,
    uint amountADesired,
    uint amountBDesired,
    uint amountAMin,
    uint amountBMin
) internal virtual returns (uint amountA, uint amountB) {
    // create the pair if it doesn't exist yet
    if (IUniswapV2Factory(factory).getPair(tokenA, tokenB) == address(0)) {
        IUniswapV2Factory(factory).createPair(tokenA, tokenB);
    }
    (uint reserveA, uint reserveB) = UniswapV2Library.getReserves(factory, tokenA, tokenB);
    if (reserveA == 0 && reserveB == 0) {
        (amountA, amountB) = (amountADesired, amountBDesired);
    } else {
        uint amountBOptimal = UniswapV2Library.quote(amountADesired, reserveA, reserveB);
        if (amountBOptimal <= amountBDesired) {
            require(amountBOptimal >= amountBMin, 'UniswapV2Router: INSUFFICIENT_B_AMOUNT');
            (amountA, amountB) = (amountADesired, amountBOptimal);
        } else {
            uint amountAOptimal = UniswapV2Library.quote(amountBDesired, reserveB, reserveA);
            assert(amountAOptimal <= amountADesired);
            require(amountAOptimal >= amountAMin, 'UniswapV2Router: INSUFFICIENT_A_AMOUNT');
            (amountA, amountB) = (amountAOptimal, amountBDesired);
        }
    }
}

function addLiquidity(
    address tokenA,
    address tokenB,
    uint amountADesired,
    uint amountBDesired,
    uint amountAMin,
    uint amountBMin,
    address to,
    uint deadline
) external virtual override ensure(deadline) returns (uint amountA, uint amountB, uint liquidity) {
    (amountA, amountB) = _addLiquidity(tokenA, tokenB, amountADesired, amountBDesired, amountAMin, amountBMin);
    address pair = UniswapV2Library.pairFor(factory, tokenA, tokenB);
    TransferHelper.safeTransferFrom(tokenA, msg.sender, pair, amountA);
    TransferHelper.safeTransferFrom(tokenB, msg.sender, pair, amountB);
    liquidity = IUniswapV2Pair(pair).mint(to);
}

The point is the deadline check

modifier ensure(uint deadline) {
    require(deadline >= block.timestamp, 'UniswapV2Router: EXPIRED');
    _;
}

The deadline check ensure that the transaction can be executed on time and the expired transaction revert.

Impact

The transaction can be pending in mempool for a long and the trading activity is very time sensitive. Without deadline check, the trade transaction can be executed in a long time after the user submit the transaction, at that time, the trade can be done in a sub-optimal price, which harms user's position.

The deadline check ensure that the transaction can be executed on time and the expired transaction revert.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding deadline check like in the functions like withdraw and deposit.

Assessed type

Invalid Validation

[c4-pre-sort]

0xEVom marked the issue as primary issue

[c4-pre-sort]

0xEVom marked the issue as sufficient quality report

[0xEVom]

Related code-423n4/2024-02-uniswap-foundation-findings#331

[c4-sponsor]

kalinbas (sponsor) acknowledged

[kalinbas]

ERC4626 does not support this parameter, so we can't add it to the main deposit and withdraw functions.

[kalinbas]

Related code-423n4/2024-02-uniswap-foundation-findings#331

Could you give more info please? I do not have access to the linked repo. @0xEVom

[kalinbas]

Also deposit and withdraw do not have any slippage problem (like trading does) so it is not needed here. This should be a QA severity.

[c4-sponsor]

kalinbas marked the issue as disagree with severity

[0xEVom]

@kalinbas yes sorry, that was more for future and internal reference. Severity of a similar issue is currently being debated.

I guess this is very similar to #281 (comment)

Note that trading is also mentioned in this finding (see #129)

[kalinbas]

Ok i agree with deadline for trading. Will add it there

[c4-sponsor]

kalinbas (sponsor) confirmed

[jhsagd76]

Based on this well-known case that has been discussed by several senior judges, QA is appropriate. code-423n4/2024-02-uniswap-foundation-findings#331

[c4-judge]

jhsagd76 changed the severity to QA (Quality Assurance)

[c4-judge]

jhsagd76 marked the issue as grade-a