Chainlink use stETH oracle and stETH rebase can have delay and impact protocol accounting

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/PriceFeed.sol#L98 (https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/BorrowerOperations.sol#L561

Vulnerability details

Impact

Chainlink use stETH oracle and stETH rebase can have delay and impact protocol accounting

Proof of Concept

the protocol has two way to value the collateral worth

1. using chainlink price feed, by calling priceFeed.fetchPrice()

which calls this line of code

  try STETH_ETH_CL_FEED.latestRoundData() returns (
        uint80 roundId,
        int256 answer,
        uint256,
        /* startedAt */
        uint256 timestamp,
        uint80 /* answeredInRound */
    ) {
        stEthEthAnswer = answer;
        chainlinkResponse.roundStEthEthId = roundId;
        chainlinkResponse.timestampStEthEth = timestamp;
    } catch {
        // If call to Chainlink aggregator reverts, return a zero response with success = false
        return chainlinkResponse;
    }

2. second way is calling

collateral.getPooledEthByShares

to handle the rebase behavior, the amount of share is tracked instead of amount of stETH is used

so to convert the exact amount of stETH, the function

collateral.getPooledEthByShares

however, stETH has a rebase mechanism, all reward or loss is socalized to all stETH holder by increasing / decreasing share value

so the collateral.getPooledEthByShares is considered most up-dated valuation

and when position rebase happens, say the worth increase from 2000 USD to 2100 USD

or

when position rebase happens, say the worth increase from 2000 USD to 1900 USD

the chainlink price feed can have delays

so the use of priceFeed.fetchPrice() to determine collateral worth can use the outdated price before the rebasing

for example, in this line of code

   function closeCdp(bytes32 _cdpId) external override {
        address _borrower = sortedCdps.getOwnerAddress(_cdpId);
        _requireBorrowerOrPositionManagerAndUpdateManagerApproval(_borrower);

        _requireCdpisActive(cdpManager, _cdpId);

        cdpManager.syncAccounting(_cdpId);

        uint256 price = priceFeed.fetchPrice();
            _requireNotInRecoveryMode(_getCachedTCR(price));

if the positive rebase already happens and but the chainlink price is not updated yet,

the price is oudated and the code can incorrectly bypass the check

_requireNotInRecoveryMode

and close cdp

Tools Used

Manual Review

Recommended Mitigation Steps

i think the protocol should use oracle consistently either use priceFeed.fetchPrice() or collateral.getPooledEthByShares across the codebase

Assessed type

Invalid Validation

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[c4-sponsor]

GalloDaSballo (sponsor) disputed

[GalloDaSballo]

We have a higher fee for this purpose, missing all the math modelling

[jhsagd76]

the purpose mentioned above can be achieved by monitoring the memory pool. But at the current interest rate, steth changes the base at about 1/10,000 per time. The attacker cannot profit, so it is to qa

[c4-judge]

jhsagd76 changed the severity to QA (Quality Assurance)