Reality Cards contest - cmichel's results
The world's first 'outcome ownership' prediction market.
General Information
Platform: Code4rena
Start Date: 19/08/2021
Pot Size: $30,000 USDC
Total HM: 5
Participants: 11
Period: 7 days
Judge: 0xean
Total Solo HM: 4
Id: 26
League: ETH
Findings Distribution
Researcher Performance
Rank: 3/11
Findings: 3
Award: $4,235.55
🌟 Selected for report: 3
🚀 Solo Findings: 1
Findings Information
Handle
cmichel
Vulnerability details
The RCTreasury.marketWhitelistCheck function gets the marketWhitelist[msgSender()] variable and performs a special check if it's non-zero.
However, there's no way to set the whitelist in the first place making this function unnecessary.
Impact
The market whitelist cannot be set and can therefore not be used.
Recommended Mitigation Steps
Implement a way to set the marketWhitelist[market] variable for a market.
#0 - Splidge
2021-08-26T11:31:12Z
Duplicate of #18
Findings Information
🌟 Selected for report: cmichel
Labels
Awards
1731.1587 USDC - $1,731.16
External Links
Handle
cmichel
Vulnerability details
There are ERC20 tokens that may make certain customizations to their ERC20 contracts.
One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().
Others are rebasing tokens that increase in value over time like Aave's aTokens (balanceOf changes over time).
Impact
The RCTreasury.deposit() function will credit more deposits than the contract actually received:
erc20.safeTransferFrom(msgSender(), address(this), _amount); user[_user].deposit += SafeCast.toUint128(_amount);
Recommended Mitigation Steps
Ensure that the erc20 token does not implement any customizations.
Alternatively, a mitigation is to measure the asset change right before and after the asset-transferring routines
#0 - Splidge
2021-08-26T11:28:45Z
The issue that keeps on giving..
Labels
Awards
155.8043 USDC - $155.80
External Links
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 } }
#0 - Splidge
2021-08-26T11:37:26Z
Duplicate of #14
#1 - 0xean
2021-09-02T01:44:49Z
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.
Handle
cmichel
Vulnerability details
The RCTreasury constructor has a comment diagram dispalying the role hierarchy.
However, the actual _setRoleAdmin setup misses the LEADERBOARD setup.
Impact
Missing roles could lead to access failures.
Recommended Mitigation Steps
Check if the LEADERBOARD role is still required, then either fix the diagram or the code.
#0 - Splidge
2021-08-26T11:32:10Z
Duplicate of #44
Findings Information
🌟 Selected for report: cmichel
Labels
Awards
577.0529 USDC - $577.05
External Links
Handle
cmichel
Vulnerability details
The RCLeaderboard.market storage variable is never used.
Instead, the MARKET role seems to be used to implement authentication.
Impact
Unused code can hint at programming or architectural errors.
Recommended Mitigation Steps
Use it or remove it.
#0 - Splidge
2021-08-26T11:34:18Z
Removed it.
#1 - Splidge
2021-09-07T11:04:12Z
Fixed here
Findings Information
🌟 Selected for report: cmichel
Labels
Awards
577.0529 USDC - $577.05
External Links
Handle
cmichel
Vulnerability details
The RCFactory._checkTimestamps function only checks that the start timestamp (_timestamps[0]) is not in the past if advancedWarning != 0.
Impact
Markets can be created that already started in the past. I'm not sure if this is intended.
Recommended Mitigation Steps
Always perform the require(_timestamps[0] >= block.timestamp, "Market opening time not set"); check, not only in the advancedWarning != 0 case.
#0 - Splidge
2021-08-26T11:36:44Z
In the case where we want to open a market immediately it would be difficult to do so without allowing timestamps in the past because we can't say what time the transaction will be mined and so what the block.timestamp would be.
By not checking this we can open markets immediately with ease.
#1 - 0xean
2021-09-02T12:03:23Z
The check could allow for some expected amount of time slippage due to mining to have transpired and potentially still be useful to avoid errors. I think it's a valid point by the warden.
#2 - Splidge
2021-09-07T11:03:31Z
Changing to acknowledged based on the judges assessment.
Labels
Awards
259.6738 USDC - $259.67
External Links
Handle
cmichel
Vulnerability details
Some parameters of functions are not checked for invalid values:
RCOrderbook.constructor:address _treasury
Impact
A wrong user input or wallets defaulting to the zero addresses for a missing input can lead to the contract needing to redeploy or wasted gas.
Recommended Mitigation Steps
Validate the parameters.
#0 - Splidge
2021-08-26T11:40:22Z
Almost a duplicate of #61 however this issue hasn't highlighted the one zero-address check that was problematic. RCOrderbook contains a setter function for the treasury so this isn't an issue.
#1 - 0xean
2021-09-02T12:06:50Z
Valid issue - the setter checks to validate the input and so should the constructor. Marking as a duplicate of #61