Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential Price Manipulation Attack due to Oracle design #381

Open
c4-submissions opened this issue Nov 14, 2023 · 6 comments
Open

Potential Price Manipulation Attack due to Oracle design #381

c4-submissions opened this issue Nov 14, 2023 · 6 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-723 grade-b Q-76 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L52

Vulnerability details

Impact

The getRSETHPrice() function calculates the RSETH/ETH exchange rate based on the total value of assets in the LRT deposit pool, derived from individual asset prices fetched by getAssetPrice(), which in turn relies on Chainlink.

The function is potentially vulnerable to price manipulation if the oracle feeds can be tampered with, or if the pool balances can be artificially inflated, such as through a flash loan attack.

This price outrages provide a substantial attack surface for the protocol therefore it's worth adding some complexity to the current implementation.

Proof of Concept

function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

        if (rsEthSupply == 0) {
            return 1 ether;
        }

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];
            uint256 assetER = getAssetPrice(asset);

            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;
    }

The oracle vulnerability can be exploited if an attacker finds a way to manipulate the data feed of the oracle service. For instance, by exploiting a vulnerability in the chainlink data feed.

An attacker could also take a substantial loan of a supported asset, deposits it into the LRT deposit pool, calls getRSETHPrice() to get an inflated price, and then uses this inflated price for profit in other transactions within the same block before repaying the flash loan.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider querying both the Chainlink oracle and Uniswap pool (TWAP implementation ) for latest prices, ensuring that these two values are within some upper/lower bounds of each other.

Slabbed under Medium risk as the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions.

Assessed type

Oracle

@c4-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 14, 2023
c4-submissions added a commit that referenced this issue Nov 14, 2023
@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Nov 16, 2023
@c4-pre-sort
Copy link

raymondfam marked the issue as sufficient quality report

@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #32

@c4-pre-sort
Copy link

raymondfam marked the issue as not a duplicate

@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #194

@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #723

@c4-judge c4-judge removed the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Dec 1, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Dec 1, 2023

fatherGoose1 changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Dec 1, 2023
@C4-Staff C4-Staff reopened this Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-723 grade-b Q-76 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants