Users can profit by calling Vault functions on the same block

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L1155

Vulnerability details

Impact

• Users can profit by calling Vault functions in the same block
• Exchange rate is not updated for actions on the same block

Proof of Concept

In contract Vault, Debt exchange rate and lend exchange rate is only calculated once per block:

    function _updateGlobalInterest()
        internal
        returns (uint256 newDebtExchangeRateX96, uint256 newLendExchangeRateX96)
    {
        // only needs to be updated once per block (when needed)
        if (block.timestamp > lastExchangeRateUpdate) {
            (newDebtExchangeRateX96, newLendExchangeRateX96) = _calculateGlobalInterest();
            lastDebtExchangeRateX96 = newDebtExchangeRateX96;
            lastLendExchangeRateX96 = newLendExchangeRateX96;
            lastExchangeRateUpdate = block.timestamp;
            emit ExchangeRateUpdate(newDebtExchangeRateX96, newLendExchangeRateX96);
        } else {
            newDebtExchangeRateX96 = lastDebtExchangeRateX96;
            newLendExchangeRateX96 = lastLendExchangeRateX96;
        }
    }

This will create issues for transactions happens in the same block, the users could either profit or suffer loss because the exchange rate is not re-calculated.
For example a user could borrow and then deposit at the same block, the user will enjoy a lower exchange rate in deposit transaction and receive more shares.

Below is a PoC for the above issue, save these 2 test cases to file test/integration/V3Vault.t.sol and run it using command:
forge test --match-path test/integration/V3Vault.t.sol --match-test testSameBlockActions -vvvv

function testSameBlockActions1() external {

        vault.setLimits(0,  150000000, 150000000, 120000000, 120000000);

        //
        _setupBasicLoan(false);
        (,, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);

        //
        vm.prank(TEST_NFT_ACCOUNT);
        vault.borrow(TEST_NFT, collateralValue);


        // Deposit again
        _deposit(10000000, WHALE_ACCOUNT);
        assertEq(vault.balanceOf(WHALE_ACCOUNT), 20000000);
    }

    function testSameBlockActions2() external {

        vault.setLimits(0,  150000000, 150000000, 120000000, 120000000);

        //
        _setupBasicLoan(false);
        (,, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);

        //
        vm.prank(TEST_NFT_ACCOUNT);
        vault.borrow(TEST_NFT, collateralValue);


        // Deposit again
        vm.warp(block.timestamp + 1);
        _deposit(10000000, WHALE_ACCOUNT);
        assertEq(vault.balanceOf(WHALE_ACCOUNT), 19999999);
    }

In test case testSameBlockActions1, borrow and deposit happens in the same block, user receive 20000000 shares while in test case testSameBlockActions2, they happen on different blocks and user only receive 19999999 shares.

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend calling _calculateGlobalInterest every time _updateGlobalInterest is called.

Assessed type

Timing

[c4-pre-sort]

0xEVom marked the issue as sufficient quality report

[c4-pre-sort]

0xEVom marked the issue as duplicate of #435

[c4-judge]

jhsagd76 changed the severity to 2 (Med Risk)

[c4-judge]

jhsagd76 marked the issue as satisfactory

[ktg9]

Hi @c4-judge , I think this issue is not a duplicate of #435.
What I demonstrated in this issue is that the exchange rates are not updated for actions in the same block -> so users can profit by doing actions on the same block.

If a user performs a series of actions like A, B, C on the same block, then C will enjoy the same exchange rates as A and B even though A and B should have updated exchanges rates.

In the PoC I demonstrated that a user gains more shares in deposit by performing action on the same block with other actions.

Can you check?

[jhsagd76]

Maintained, there is no misunderstanding.