Platform: Code4rena
Start Date: 10/06/2021
Pot Size: $45,000 USDC
Total HM: 21
Participants: 12
Period: 7 days
Judge: LSDan
Total Solo HM: 13
Id: 13
League: ETH
Rank: 9/12
Findings: 2
Award: $433.62
🌟 Selected for report: 2
🚀 Solo Findings: 0
237.3183 USDC - $237.32
jvaqa
RCFactory.createMarket() does not enforce _timestamps[1] and _timestamps[2] being larger than _timestamps[0], even though proper functioning requires them to be so.
IRCMarket defines a sequence of events that each market should progress through sequentially, CLOSED, OPEN, LOCKED, WITHDRAW. // [1]
The comments explicitly state that _incrementState() should be called "thrice". // [2]
However, it is possible to create a market where these events do not occur sequentially.
You can create a market where the marketOpeningTime is later than the marketLockingTime and oracleResolutionTime.
This is because although RCFactory checks to ensure that _timestamps[2] is greater than _timestamps[1], it does not check to ensure that _timestamps[1] is greater than _timestamps[0]. // [3]
This is also because although RCFactory checks to ensure that _timestamps[0] is equal to or greater than block.timestamp, it makes no check for a minimum value for _timestamps[1] or _timestamps[2], or a relative check between the value of _timestamps[0] and _timestamps[1]. // [4]
Thus, you can create a market where the marketLockingTime and the oracleResolutionTime occur before the marketOpeningTime.
When calling RCFactory.createMarket(), Alice can supply 0 as the argument for _timestamps[1] and _timestamps[2], and any value equal to or greater than block.timestamp for _timestamps[0]. // [5]
Add the following check to RCFactory.createMarket():
require( _timestamps[0] < _timestamps[1], "market must begin before market can lock" );
#0 - Splidge
2021-06-21T10:42:21Z
Implemented here
79.1061 USDC - $79.11
jvaqa
According to Solidity documentation for v0.8.0:
"Properly functioning code should never [trigger an assert], not even on invalid external input. If this happens, then there is a bug in your contract which you should fix."[1] [1] https://docs.soliditylang.org/en/v0.8.0/control-structures.html#error-handling-assert-require-revert-and-exceptions
The reason that asserts should never be triggered in bug-free code is that triggering an assert() causes the transaction to use up more space on the blockchain than is necessary, since the transaction uses all gas provided by the user in the transaction's gasLimit, whereas triggering a require() only uses up the gas used up to the point of the failed require() statement. Using up more gas than necessary hurts the public good of blockspace, by unnecessarily filling up the block with more gas than is strictly necessary for the failed transaction to use. It is useful in development environments to distinguish between allowable reverts and unallowable reverts in automated testing, but asserts should not be reached in live code.
Alice can call RCTreasury.collectRentUser(aliceAddress, 0) to trigger an assert
Change:
assert(_timeToCollectTo != 0);
To:
require(_timeToCollectTo != 0);
#0 - Splidge
2021-06-17T11:30:32Z
Duplicate of #155
#1 - dmvt
2021-07-11T10:24:51Z
duplicate of #83
117.1942 USDC - $117.19
jvaqa
RCTreasury.addToWhitelist() will erroneously remove user from whitelist if user is already whitelisted
The comments state that calling addToWhitelist() should add a user to the whitelist. [1]
However, since the implementation simply flips the user's whitelist bool, if the user is already on the whitelist, then calling addToWhitelist() will actually remove them from the whitelist. [2]
Since batchAddToWhitelist() will repeatedly call addToWhitelist() with an entire array of users, it is very possible that someone could inadvertently call addToWhitelist twice for a particular user, thereby leaving them off of the whitelist. [3]
If a governor calls addToWhitelist() with the same user twice, the user will not be added to the whitelist, even though the comments state that they should.
Change addToWhitelist to only ever flip a user's bool to true. To clarify the governor's intention, create a corresponding removeFromWhitelist and batchRemoveFromWhitelist which flip a user's bool to false, so that the governor does not accidently remove a user when intending to add them.
Change this:
isAllowed[_user] = !isAllowed[_user]; // [4]
To this:
isAllowed[_user] = true; // [4]
And add this:
/// @notice Remove a user to the whitelist function removeFromWhitelist(address _user) public override { IRCFactory factory = IRCFactory(factoryAddress); require(factory.isGovernor(msgSender()), "Not authorised"); isAllowed[_user] = false; } /// @notice Remove multiple users from the whitelist function batchRemoveFromWhitelist(address[] calldata _users) public override { for (uint256 index = 0; index < _users.length; index++) { removeFromWhitelist(_users[index]); } }
#0 - Splidge
2021-06-15T09:51:22Z
The whitelist is only really for the initial beta phase so we aren't going to be putting more time and effort into changes here.
I think the severity can be reduced as the whitelist only limits access to deposit(), if a user was added and then erroneously removed they can still partake in the events and withdraw their winnings. It is only limiting their entry into the system.
#1 - Splidge
2021-07-08T13:46:28Z
An update, We now implement OpenZeppelin AccessControl.sol, so whitelisting is now either granting or revoking the role WHITELIST.
#2 - dmvt
2021-07-10T14:36:04Z
I don't think this is a medium severity issue as calling it a third time will toggle it back on. That said, the naming is confusing and could result in upset users / some reputational damage, particularly if the sponsor changes their mind and keeps this in place past beta. Changing severity to 1