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

getMarketInfo skipResults does not work #53

Closed
code423n4 opened this issue Aug 25, 2021 · 2 comments
Closed

getMarketInfo skipResults does not work #53

code423n4 opened this issue Aug 25, 2021 · 2 comments
Labels
2 (Med Risk) bug Something isn't working disagree with severity duplicate This issue or pull request already exists

Comments

@code423n4
Copy link
Contributor

Handle

cmichel

Vulnerability details

The RCFactory.getMarketInfo function uses the same counter _resultNumber for the result arrays' index.
This counter is increased if _skipResults is set, and the arrays are therefore not indexed at zero.

if (_resultNumber < _skipResults) {
    // @audit increases the array index
    _resultNumber++;
} else {
    // @audit will start at a higher number if _skipResults is set
    _marketAddresses[_resultNumber] = _market;
    _ipfsHashes[_resultNumber] = ipfsHash[_market];
    _slugs[_resultNumber] = addressToSlug[_market];
    _potSizes[_resultNumber] = IRCMarket(_market)
        .totalRentCollected();
    _resultNumber++;
}

Imagine _skipResults = marketInfoResults to receive the second "page" of market infos. The function will just return an empty array of size marketInfoResults because of the while(_resultNumber < marketInfoResults) condition and increasing this same counter when skipping results.

Impact

The function does not return the correct market infos if _skipResults is used.

Recommended Mitigation Steps

The _resultNumber which is the index to the result arrays may not be increased when skipping results, instead a different counter should be used.
The easiest way to fix this is by just decrementing the _skipResults variable itself.
Change the if (_resultNumber < _skipResults) condition to:

if (IRCMarket(_market).state() == IRCMarket.States(_state)) {
    if (_skipResults > 0) {
        _skipResults--;
    } else {
        // same old
    }
}
@code423n4 code423n4 added 2 (Med Risk) bug Something isn't working labels Aug 25, 2021
code423n4 added a commit that referenced this issue Aug 25, 2021
@Splidge Splidge added duplicate This issue or pull request already exists disagree with severity labels Aug 26, 2021
@Splidge
Copy link
Collaborator

Splidge commented Aug 26, 2021

Duplicate of #14

@Splidge Splidge marked this as a duplicate of #14 Aug 26, 2021
@Splidge Splidge closed this as completed Aug 26, 2021
@0xean
Copy link
Collaborator

0xean commented Sep 2, 2021

going with the severity of the duplicate #14 on this issue. Based on the functions purpose the potential blast radius of the issue does seem small.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) bug Something isn't working disagree with severity duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

3 participants