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

RCFactory: Solve stack too deep for getMarketInfo() #41

Open
code423n4 opened this issue Aug 24, 2021 · 2 comments
Open

RCFactory: Solve stack too deep for getMarketInfo() #41

code423n4 opened this issue Aug 24, 2021 · 2 comments
Labels
1 (Low Risk) bug Something isn't working Resolved Used when a fix has been implemented. sponsor confirmed

Comments

@code423n4
Copy link
Contributor

Handle

hickuphh3

Vulnerability details

Impact

The marketInfoResults is a parameter used by getMarketInfo() to determine the length of results to return. As the setMarketInfoResults() comments state, "(it) would be better to pass this as a parameter in getMarketInfo.. however we are limited because of stack too deep errors".

This limitation can be overcome by defining the return array variables as the function output, as suggested below.

The need for marketInfoResults and its setter function is then made redundant, whilst making querying results of possibly varying lengths more convenient.

Recommended Mitigation Steps

function getMarketInfo(
  IRCMarket.Mode _mode,
  uint256 _state,
  uint256 _skipResults,
  uint256 _numResults // equivalent of marketInfoResults
)
  external
  view
  returns (
    address[] memory _marketAddresses,
    string[] memory _ipfsHashes,
    string[] memory _slugs,
    uint256[] memory _potSizes
	)
 {
	  uint256 _marketIndex = marketAddresses[_mode].length;
	  
	  _marketAddresses = new address[](_numResults);
	  _ipfsHashes = new string[](_numResults);
	  _slugs = new string[](_numResults);
	  _potSizes = new uint256[](_numResults);
		...
}
@code423n4 code423n4 added 1 (Low Risk) bug Something isn't working labels Aug 24, 2021
code423n4 added a commit that referenced this issue Aug 24, 2021
@Splidge
Copy link
Collaborator

Splidge commented Aug 25, 2021

I wish I saw this before I did the fix to issue #14 , in implementing that fix I split getMarketInfo() in half making part of it internal which has managed to get around the "Stack too deep" problem and so I was able to get rid of marketInfoResults at the same time.
Still this is useful to keep in mind for the future. 👍

@Splidge Splidge added Resolved Used when a fix has been implemented. sponsor confirmed labels Aug 25, 2021
@Splidge
Copy link
Collaborator

Splidge commented Sep 7, 2021

Fixed alongside issue #14 here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 (Low Risk) bug Something isn't working Resolved Used when a fix has been implemented. sponsor confirmed
Projects
None yet
Development

No branches or pull requests

2 participants