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

Binary search can overflow (very unlikely for this contract) #44

Open
3esmit opened this issue Dec 12, 2023 · 2 comments
Open

Binary search can overflow (very unlikely for this contract) #44

3esmit opened this issue Dec 12, 2023 · 2 comments

Comments

@3esmit
Copy link

3esmit commented Dec 12, 2023

In minime, the function balanceOfAt, or totalSupplyAt, uses a binary search to look for history of value in a certain block height.
The implementation uses a binary search calculation that could overflow when searching in very large data sets.

https://github.com/vacp2p/minime/blob/1f6820c2450553a4c9a600a51c727619e68f0c02/contracts/MiniMeBase.sol#L424C28-L424C42

However, this is unlikely due the size of uint256, and we dont report bugs for when the size of this arrays gets filled up.

Anyway, I think this should be fixed because the code for binary search for sake of correctness, as any smart contract, no bugs should be allowed, and also this could be copied to use in another contract where this might not be the case.

Currently this is the implementation:
uint256 mid = (high + low + 1) / 2;

The correct way should be:
uint256 mid = low + ((high - low + 1) / 2);

@0x-r4bbit
Copy link
Collaborator

Interesting.
I wonder if this is something the certora prover could find, given a proper spec

@0x-r4bbit 0x-r4bbit self-assigned this Feb 8, 2024
@0x-r4bbit
Copy link
Collaborator

Unassigning as w're not working on this atm.

@0x-r4bbit 0x-r4bbit removed their assignment May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

2 participants