Chainlink's latestRoundData might return stale or incorrect results

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/EUSDMiningIncentives.sol#L151
https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/EUSDMiningIncentives.sol#L152
https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/EUSDMiningIncentives.sol#L212

Vulnerability details

Impact

On EUSDMiningIncentives.sol, the team is using latestRoundData, but there is no check if the return value indicates stale data.

    function stakedLBRLpValue(address user) public view returns (uint256) {
        uint256 totalLp = IEUSD(ethlbrLpToken).totalSupply();
        (, int etherPrice, , , ) = etherPriceFeed.latestRoundData();
        (, int lbrPrice, , , ) = lbrPriceFeed.latestRoundData();
        uint256 etherInLp = (IEUSD(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2).balanceOf(ethlbrLpToken) * uint(etherPrice)) / 1e8;
        uint256 lbrInLp = (IEUSD(LBR).balanceOf(ethlbrLpToken) * uint(lbrPrice)) / 1e8;
        uint256 userStaked = IEUSD(ethlbrStakePool).balanceOf(user);
        return (userStaked * (lbrInLp + etherInLp)) / totalLp;
    }

....

    function purchaseOtherEarnings(address user, bool useEUSD) external updateReward(user) {
        require(isOtherEarningsClaimable(user), "The rewards of the user cannot be bought out");
        if(useEUSD) {
            require(isEUSDBuyoutAllowed, "The purchase using EUSD is not permitted.");
        }
        uint256 reward = rewards[user];
        if (reward > 0) {
            rewards[user] = 0;
            uint256 biddingFee = (reward * biddingFeeRatio) / 10000;
            if(useEUSD) {
                (, int lbrPrice, , , ) = lbrPriceFeed.latestRoundData();
                biddingFee = biddingFee * uint256(lbrPrice) / 1e8;
                bool success = EUSD.transferFrom(msg.sender, address(configurator), biddingFee);
                require(success, "TF");
                try configurator.distributeRewards() {} catch {}
            } else {
                IesLBR(LBR).burn(msg.sender, biddingFee);
            }
            IesLBR(esLBR).mint(msg.sender, reward);

            emit ClaimedOtherEarnings(msg.sender, user, reward, biddingFee, useEUSD, block.timestamp);
        }
    }

This could lead to stale prices according to the Chainlink documentation:

https://docs.chain.link/docs/historical-price-data/#historical-rounds
https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Proof of Concept

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/EUSDMiningIncentives.sol#L151

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/EUSDMiningIncentives.sol#L152

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/EUSDMiningIncentives.sol#L212

Tools Used

Manual Review

Recommended Mitigation Steps

    function stakedLBRLpValue(address user) public view returns (uint256) {
        uint256 totalLp = IEUSD(ethlbrLpToken).totalSupply();

        (uint80 roundID, int256 etherPrice, , uint256 timestamp, uint80 answeredInRound) = etherPriceFeed.latestRoundData();
        require(answeredInRound >= roundID, "Stale price");
        require(timestamp != 0,"Round not complete");
        require(etherPrice > 0,"Chainlink answer reporting 0");

        (uint80 roundID1, int256 lbrPrice, , uint256 timestamp1, uint80 answeredInRound1) = lbrPriceFeed.latestRoundData();
        require(answeredInRound1 >= roundID, "Stale price");
        require(timestamp1 != 0,"Round not complete");
        require(lbrPrice > 0,"Chainlink answer reporting 0");

        uint256 etherInLp = (IEUSD(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2).balanceOf(ethlbrLpToken) * uint(etherPrice)) / 1e8;
        uint256 lbrInLp = (IEUSD(LBR).balanceOf(ethlbrLpToken) * uint(lbrPrice)) / 1e8;
        uint256 userStaked = IEUSD(ethlbrStakePool).balanceOf(user);
        return (userStaked * (lbrInLp + etherInLp)) / totalLp;
    }

....

    function purchaseOtherEarnings(address user, bool useEUSD) external updateReward(user) {
        require(isOtherEarningsClaimable(user), "The rewards of the user cannot be bought out");
        if(useEUSD) {
            require(isEUSDBuyoutAllowed, "The purchase using EUSD is not permitted.");
        }
        uint256 reward = rewards[user];
        if (reward > 0) {
            rewards[user] = 0;
            uint256 biddingFee = (reward * biddingFeeRatio) / 10000;
            if(useEUSD) {
                (uint80 roundID, int256 lbrPrice, , uint256 timestamp, uint80 answeredInRound) = lbrPriceFeed.latestRoundData();
                require(answeredInRound >= roundID, "Stale price");
                require(timestamp != 0,"Round not complete");
                require(lbrPrice > 0,"Chainlink answer reporting 0");

                biddingFee = biddingFee * uint256(lbrPrice) / 1e8;
                bool success = EUSD.transferFrom(msg.sender, address(configurator), biddingFee);
                require(success, "TF");
                try configurator.distributeRewards() {} catch {}
            } else {
                IesLBR(LBR).burn(msg.sender, biddingFee);
            }
            IesLBR(esLBR).mint(msg.sender, reward);

            emit ClaimedOtherEarnings(msg.sender, user, reward, biddingFee, useEUSD, block.timestamp);
        }
    }

Assessed type

Oracle

[c4-pre-sort]

JeffCX marked the issue as primary issue

[c4-sponsor]

LybraFinance marked the issue as disagree with severity

[0xean]

@LybraFinance - can you explain why you disagree with the severity here?

[c4-judge]

0xean marked the issue as satisfactory

[LybraFinance]

Because we use Chainlink price feeds to report LBR price only in specific places, even if there is a delay, the impact is minimal. Moreover, the likelihood of delays occurring is low, so we consider this to be a low-risk scenario.

[0xean]

I understand your point, but do think validating the values being returned from chainlink is best practice and in the past has qualified as M severity in c4 contests. I can see how it would be considered a leak of value in the scenario's being used, M seems reasonable but I am open to further discussion.

[0xean]

This was actually found during the bot race, closing as out of scope

[c4-judge]

0xean marked the issue as unsatisfactory:
Out of scope

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-c