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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added some tests to confirm Oracle price manipulation. #2

Closed
wants to merge 2 commits into from

Conversation

0xfps
Copy link

@0xfps 0xfps commented Mar 18, 2023

Hi, my name is Anthony, wonderful code you got here, I love it, however, I made a little realization.

Since the oracle price returns int256, I think that these values can be manipulated by an attacker to return values < 0, where by when cast to uint256 will overflow causing the price to be > 1,000,000 as specified in the code.
(This can be used in favour of or against USDC.)

I suggest making sure that returned prices are >= 0 before casting to uint256, to be safe.

Reference:
https://github.com/Anish-Agnihotri/Hyperbitcoinization/blob/master/src/Hyperbitcoinization.sol#L125-L129

My Recommentation:

function getBTCPrice() public view returns (uint256) {
    // Collect BTC price
    (, int256 price,,,) = BTCUSD_PRICEFEED.latestRoundData();
    if (price < 0) revert CUSTOM_ERROR();
    return uint256(price) / 10 ** BTCUSD_PRICEFEED.decimals();
}

PS: I whopped up a fake Oracle for this test.
Location: test/myFolder/AnotherHyperbitcoinization.t.sol

I am open to corrections if any.

Once again, wonderful contract, great job 馃憤馃従!

@Anish-Agnihotri
Copy link
Owner

Thanks Anthony, I appreciate your help.

Had a decent diff from the last few changes when I saw this, so added in here.

@0xfps
Copy link
Author

0xfps commented Mar 18, 2023

Thanks Anthony, I appreciate your help.

Had a decent diff from the last few changes when I saw this, so added in here.

Much appreciated, Anish 馃憤馃従.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants