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: 1/12
Findings: 12
Award: $14,325.87
🌟 Selected for report: 22
🚀 Solo Findings: 5
0xRajeev
The Market sponsor(address _sponsorAddress, uint256 _amount) function is an externally callable function that is specified to be callable only by the Factory contract during market creation, as documented in the Natspec @dev comment: "called by Factory during market creation.” However, there is no access control on the caller address as done in other functions.
Impact: Anyone can call with any _sponsorAddress that has given approval to treasury contract (i.e. all users across all markets who have made deposits) and make them lose that amount towards the market pot. It is not clear if an attacker could selectively do this for markets when they are winning to benefit from such funds sponsored towards the market.
Exploit Scenario: User Alice has deposited 10000 tokens to be used for different prediction markets and thereby has given Treasury contract the required allowance to spend this deposit. Evil Eve calls Market::sponsor(Alice’s address, 10,000) and contributes Alice’s deposit amount towards the market pot liquidity which cannot be won back. Alice loses all her deposit to the market which may possibly benefit Eve.
Manual Analysis
Add require(msgSender() == factory.owner(), "Not authorised”); to Market::sponsor().
#0 - Splidge
2021-06-17T13:53:59Z
Duplicate of #40
#1 - dmvt
2021-07-11T12:39:47Z
duplicate of #40
0xRajeev
As specified, uberOwners of Factory, Orderbook and Treasury have the highest privileges in the system because they can upgrade contracts of market, Nfthub, order book, treasury, token and factory which form the critical components of the protocol.
The contracts allow for uberOwners to be changed to a different address from the contract owner/deployer using the changeUberOwner() function which is callable by the current uberOwner. While this function checks for zero-address, there is no validation of the new address being correct. If the current uberOwner incorrectly uses an invalid address for which they do not have the private key, then the system gets locked because the uberOwner cannot be corrected and none of the other functions that require uberOwner caller can be executed.
Impact: The current uberOwner uses a non-zero but incorrect address as the new uberOwner. This gets set and now the system is locked and none of the uberOwner-only callable functions are callable. This error cannot be fixed either and will require redeployment of contracts which will mean that all existing markets have to be terminated. The system will have to be shut and restarted completely from scratch which will take a reputation hit and have a serious technical and business impact.
https://github.com/code-423n4/2021-06-realitycards#mortar_board-governance-mortar_board
Manual Analysis
Change the single-step change of uberOwner address to a two-step process where the current uberOwner first approves a new address as a pendingUberOwner. That pendingUberOwner has to then claim the ownership in a separate transaction which cannot be done if they do not have the correct private key. An incorrectly set pendingUberOwner can be reset by changing it again to the correct one who can then successfully claim it in the second step.
#0 - Splidge
2021-06-17T14:41:59Z
Duplicate of #5
#1 - dmvt
2021-07-09T13:28:20Z
There is a very low probability coupled with a very high impact, making this a Medium risk issue in my opinion.
351.5826 USDC - $351.58
0xRajeev
The balancedBooks modifier is used to “check that funds haven't gone missing during this function call” and is applied to deposit, withdrawDeposit, payRent, payout and sponsor Treasury functions which move funds in and out of the Treasury or adjust its market/user balances.
However, this modifier is missing on refundUser() and topupMarketBalance() functions which also perform similar actions.
Impact: Any miscalculations in these functions will lead to the system becoming insolvent.
Manual Analysis
Add modifier to the two functions above where it is missing.
#0 - Splidge
2021-06-18T12:34:23Z
Duplicate of #23
0xRajeev
Three timestamps are used during market creation to indicate market opening, locking and oracle resolution times. The validation of these timestamps is performed in RCFactory during market creation.
The validation for market locking _timestamp[1] is performed only if maximumDuration is != 0 where the default value is 0 and is set only if setMaximumDuration() is called. Even in that validation, the use of block.timestamp should really be _timestamps[0] (market opening timestamp) because the market opening-locking duration should be counted from _timestamp[0] and not block.timestamp at createMarket() time. _timestamp[0] could be in the past. This check also doesn't account for advancedWarning aspect of market opening.
Impact: Incorrect/Insufficient validations of all possible market opening/locking configurations could affect user interactions with the market by breaking assumed invariants in the code.
Manual Analysis
Specify all possible market opening/locking invariants and validate correctly against all variations of them.
#0 - Splidge
2021-06-18T10:17:14Z
Duplicate of #61
#1 - dmvt
2021-07-10T13:44:03Z
duplicate of #61
🌟 Selected for report: 0xRajeev
1302.1578 USDC - $1,302.16
0xRajeev
rentAllCards() requires the sender to specify a _maxSumOfPrices parameter which specifies “limit to the sum of the bids to place” as specified in the Natspec @param comment. This is apparently for front-run protection.
However, this function parameter constraint for _maxSumOfPrices is broken in the function implementation which leads to the total of bids places greater than the _maxSumOfPrices specified.
Impact: The user may not have sufficient deposit, be foreclosed and/or impacted on other bids/markets.
Scenario: Assume two cards for a market with current winning rentals of 50 each. _maxSumofPrices = 101 passes check on L643 but then the forced 10% increase on L650 (assuming sender is not the owner of either card) causes newRentals to be called with 55 for each card thus totalling to 110 which is > 101 as requested by the user.
Manual Analysis
Modify the max sum of prices check logic to consider the 10% increase scenarios. Document and suggest the max sum of prices for the user in the UI based on the card prices and 10% requirement depending on card ownership.
#0 - Splidge
2021-06-21T11:11:36Z
fixed here
🌟 Selected for report: 0xRajeev
1302.1578 USDC - $1,302.16
0xRajeev
Ability to pause all token transfers and all state changes for contracts is a “guarded-launch” best-practice for emergency situations for newly launched projects. The project implements this using a marketsPaused flag per market and a globalPause flag across all markets.
While these prevent renting of cards in a specific market and deposit/withdraw/rent cards across all markets, there are still public/external functions that are unguarded by these flags which can affect contract state in paused scenarios that will make it hard/impossible to recover correctly from the emergency pause.
Examples: topupMarketBalance() and refundUser() in Treasury can be triggered even in a globalPause scenario. There could be other function flows where it is not obvious that market/global pausing is enabled because it is enforced in one of the functions called deep within the code within one of the conditionals.
Impact: Markets get paused but the contracts cannot be restarted because of state changes affected during the pause via unguarded external/public functions.
Manual Analysis
Apply marketPaused and globalPause check clearly in the beginning of all public/external functions which move tokens/funds in/out or change contract state in any way.
Validate all possible control flows to check that market/global pausing works in all scenarios and applies to all contract state and not specific functionalities.
#0 - Splidge
2021-06-18T11:58:27Z
marketPause is declared as only limiting rentals in a specific market.
globalPause is declared as stopping deposits, withdraws and rentals across all markets.
Therefore they are functioning as intended.
Also, the example of refundUser() is not true, it will fail in a globalPause because it is only called by markets during a rent collection and a rent collection requires the calling of payout which is restricted by globalPause.
#1 - dmvt
2021-07-10T16:56:43Z
topupMarketBalance does appear to be a deposit of sorts. I think the warden's take on the issue is valid and sponsor should seriously consider looking closer at the potential side effects of not fully pausing intentional transfer functions.
🌟 Selected for report: 0xRajeev
1302.1578 USDC - $1,302.16
0xRajeev
The Treasury deposit() function credits amount to the user address in parameter instead of the msgSender() that is actually making the deposit whose rationale as explained in the Natspec comment is that this may be called via contract or L1-L2 bot.
However, the deposit whitelist should ideally be enforced on the _user address. If msgSender() is blacklisted, user address can still deposit() from another whitelisted msgSender() address while retaining the user address that is used for leader boards and NFTs.
Impact: User misbehaves in interactions with the system (e.g. trolls, spams) and their corresponding msgSender() is removed from the whitelist. User continues to deposit into the system via another whitelisted msgSender() without any impact to leader boards or NFTs.
Manual Analysis
Use whitelist on user address instead of msgSender().
#0 - Splidge
2021-06-18T13:34:56Z
It is stated that the whitelist will "only allow certain addresses to deposit" here and that toggleWhitelist() allows an address to deposit here.
I think that the whitelist is performing as intended, but thanks for this issue report as this could easily have been a larger issue.
We only plan to use the whitelist as a very rudimentary barrier just for the initial launch. I think that only allowing certain addresses to deposit is sufficient for now. Maybe if time allows I'll make the changes but changing the whitelist to allow the _user instead of the msgSender() would also block contracts and layer1->layer2 bot, so there'd need to be exceptions made for them. I'd rather not play about with sensitive functions at the last minute when we aren't going to be using the whitelist much anyway.
#1 - dmvt
2021-07-10T17:28:04Z
Warden makes a good point. This could allow griefing of other parts of the system. If the barrier winds up being needed longer than expected or users act in unexpected ways, sponsor may wind up wishing they had reconsidered addressing this. Obviously, sponsor is free to ignore, but the issue seems to be a valid one with significant potential impact.
🌟 Selected for report: 0xRajeev
1302.1578 USDC - $1,302.16
0xRajeev
Orderbook.removeBids() as commented “///remove bids in closed markets for a given user ///this can reduce the users bidRate and chance to foreclose”
removeOldBids() is performed currently in Market.newRental() and Treasury.deposit() to “do some cleaning up, it might help cancel their foreclosure” as commented. However, this is missing in the withdrawDeposit() function where the need is the most because user is removing deposit which may lead to foreclosure and is even commented as being useful on L356.
Impact: If we do not remove closed market bids during withdrawDeposit, the closed market bids still get accounted in user's bidrate in the conditional on L357 and therefore do not prevent the foreclosure in withdrawDeposit that may happen in L357-L367. User may get foreclosed because of mis-accounted closed-market bids in the order book.
Manual Analysis
Add call to removeOldBids() on L355 of withdrawDeposit() of Treasury.
#0 - Splidge
2021-06-21T11:32:22Z
This was intentionally left out in an older version of the contracts because of the way withdrawDeposit worked before we had the per-user rent collection.
Added it back in again here.
🌟 Selected for report: 0xRajeev
1302.1578 USDC - $1,302.16
0xRajeev
ERC721 standard and implementation allows the use of approved addresses to affect transfers besides the token owners. However, the L2 NFT Hub implementation deviates from ERC721 by ignoring the presence of any approvers in the overriding function implementations of transferFrom() and safeTransferFrom().
Impact: The system interactions with NFT platforms may not work if they expect ERC721 adherence. Users who interact via approved addresses will see their transfers failing for their approved addresses.
Given that the key value proposition of this project is the use of NFTs, the expectation will be that it is fully compatible with ERC721.
Manual Analysis
Add support for approval in NFT transfers.
#0 - mcplums
2021-06-17T07:17:50Z
This is a nice one, I see no reason why we can't implement this
#1 - Splidge
2021-06-18T09:23:01Z
Yes, we will need to add this, although we will need to override the approvals until the market has locked and the cards true owner is discovered.
#2 - Splidge
2021-06-21T13:07:57Z
I've changed from overriding specific functions which could be dangerous if we were to upgrade to an OpenZeppelin implementation that had alternative transfer functions.
Now we use the _beforeTokenTransfer hook and check that only the factory or the market can do a transfer before the market has entered the withdraw state. Implemented here
0xRajeev
ERC721 specification for safeTransferFrom says: “this function checks if _to is a smart contract (code size > 0). If so, it calls onERC721Received on _to and throws if the return value is not bytes4(keccak256(“onERC721Received(address,address,uint256,bytes)”)).” This is implemented by the widely-used OpenZeppelin implementation which is also inherited here.
However, the overriding implementation in L2 NFT Hub is missing the check implemented by “require(_checkOnERC721Received(..))” after _transfer() to evaluate if the destination contract was indeed ERC-721 compatible.
Impact: NFT is transferred to a non ERC-721 compatible contract and gets locked/lost.
https://eips.ethereum.org/EIPS/eip-721
Manual Analysis
Add require(_checkOnERC721Received(..))” after _transfer() on L232.
#0 - Splidge
2021-06-18T10:39:17Z
Duplicate of #160
#1 - dmvt
2021-07-11T11:03:36Z
duplicate of #160
0xRajeev
Contracts use assert() instead of require() in multiple places. This causes a Panic error on failure and prevents the use of error strings.
Prior to solc 0.8.0, assert() used the invalid opcode which used up all the remaining gas while require() used the revert opcode which refunded the gas and therefore the importance of using require() instead of assert() was greater. However, after 0.8.0, assert() uses revert opcode just like require() but creates a Panic(uint256) error instead of Error(string) created by require(). Nevertheless, Solidity’s documentation says:
"Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.”
whereas
“The require function either creates an error without any data or an error of type Error(string). It should be used to ensure valid conditions that cannot be detected until execution time. This includes conditions on inputs or return values from calls to external contracts.”
Also, you can optionally provide a message string for require, but not for assert.
Manual Analysis
Use require() with informative error strings instead of assert().
#0 - Splidge
2021-06-18T10:22:16Z
Duplicate of #155
🌟 Selected for report: JMukesh
Also found by: 0xRajeev, a_delamo, cmichel, maplesyrup
0xRajeev
Zero-address checks for all address parameters is a best practice. This is especially true for constructor parameters that do not have setters to later correct any accidental use of zero-address or incorrect address.
The childChainManager parameter used in RCNftHubL2 constructor has no zero-address validation but is used as an argument for OpenZeppelin’s _setupRole() which also does not perform zero-address validation. There is no setter function to fix this later.
Impact: Using a zero-address accidentally will result in undefined behavior and require L2 hub redeployment.
Manual Analysis
Add zero-address check for childChainManager parameter used in RCNftHubL2 constructor.
#0 - Splidge
2021-06-17T12:40:30Z
It is a requirement for Matic mintable assets that the childChainManager is not alterable, so it would not be possible to have a setter for this.
#1 - Splidge
2021-06-17T12:40:49Z
Duplicate of #56
#2 - dmvt
2021-07-11T10:36:18Z
duplicate of #56
195.3237 USDC - $195.32
0xRajeev
If _collectRent() counter runs over maxRentIterations then it will return False which will make collectRentAllCards() return false in which case, the market is never locked and cards never transferred in lockMarket().
Given that maxRentIterations is a heuristic that depends on gas profiling/usage and is currently set to 35, the probability of this scenario happening is unclear but if it does happen then the impact of market not locking is significant.
Manual Analysis
Evaluate the probability of this scenario and set maxRentIterations appropriately or consider locking the market even otherwise.
#0 - Splidge
2021-06-18T10:58:00Z
the market is never locked
It will eventually be locked on subsequent calls.
The problem with locking the market before the rent calculations are performed is that the longest owner can already move the NFT at this stage, we need to finish the rent calculations before we can confirm who is the longest owner.
#1 - Splidge
2021-06-18T10:58:50Z
I'm not sure this is severity 2? I think it should be lower, it's simply having to perform the appropriate accounting before the market can be locked.
#2 - Splidge
2021-06-18T11:06:16Z
This is almost a duplicate of #12 , it has the same core to it, however unlike #12 this issue doesn't go further to explain exactly why this is a problem. It also suggests a solution that would introduce larger problems. I'll be marking this as disputed because standing alone this simply isn't an issue.
#3 - dmvt
2021-07-09T13:21:11Z
I'm going to split the middle here and mark this as a duplicate of #12
#4 - dmvt
2021-07-11T10:39:29Z
duplicate of #12
79.1061 USDC - $79.11
0xRajeev
In the deposit function, the deposit _amount has already been added to the user's deposit on L303. The addition of _amount again to the deposit on L309 for checking against daily bidRate effectively leads to double counting of deposited _amount and may keep/bring user out of foreclosure even though they are not.
Scenario: Alice’s current daily bidRate is 500 and deposit is 350. She makes a new deposit of 100 which should not bring her out of foreclosure because the new effective deposit will be 300+150 = 450 which is still less than 500. However, because of the double-counting miscalculation, the check performed is 450+100 > 500 which will pass and Alice is not foreclosed. She effectively gains double the deposit amount in treatment of deposits against foreclosure.
Manual Analysis
Change the conditional predicate on L309-310 from: user[_user].deposit + _amount > user[_user].bidRate / minRentalDayDivisor to: user[_user].deposit > user[_user].bidRate / minRentalDayDivisor
#0 - Splidge
2021-06-18T13:43:28Z
Duplicate of #26
#1 - dmvt
2021-07-09T15:22:02Z
I generally think the impact of this on the rest of the system is minimal. It results in a slight advantage to the user in foreclosure, but does not cause a loss or granting of additional funds. To take advantage of this exploit, the user would also need to be highly skilled at reading source code to find the exploit in the first place. Even if they took the time to do this, the effect would not be permanent. I'm aligned with the view expressed in #26 and #37 that this is low severity.
195.3237 USDC - $195.32
0xRajeev
The ecrecover function is used to verify and execute Meta transactions. The built-in EVM precompile ecrecover is susceptible to signature malleability (because of non-unique s and v values) which could lead to replay attacks (references: https://swcregistry.io/docs/SWC-117, https://swcregistry.io/docs/SWC-121 and https://medium.com/cryptronics/signature-replay-vulnerabilities-in-smart-contracts-3b6f7596df57).
While this is not exploitable for replay attacks in the current implementation because of the use of nonces, this may become a vulnerability if used elsewhere.
Manual Analysis
Consider using OpenZeppelin’s ECDSA library (which prevents this malleability) instead of the built-in function: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/cryptography/ECDSA.sol
#0 - Splidge
2021-06-18T10:33:30Z
This issue will now be fixed, not because of the reasons discovered in this issue, but because of the reasons explained in #166
#1 - Splidge
2021-06-18T14:45:38Z
Nevermind, issue #166 couldn't be solved using the recommended mitigation so this issue will remain unresolved for now.
🌟 Selected for report: 0xRajeev
434.0526 USDC - $434.05
0xRajeev
The current block gas limit is 15M and not 12.5 as indicated in the comment for setNFTMintingLimit(60) in Factory’s constructor. So this could be changed accordingly but a safe threshold needs to be enforced in the setter setNFTMintingLimit() which is currently lacking. That would prevent accidentally setting the minting limit to something beyond what the block gas limit would safely allow.
Manual Analysis
If NFT minting limit dependence on block gas limit is critical to the functioning, consider using GASLIMIT opcode to dynamically check block gas limit to set nftMintingLimit appropriately before creating a market.
#0 - Splidge
2021-06-17T08:51:36Z
The minting limit isn't critical to the functioning, 60 is already far beyond what we anticipate is required. The contracts will actually be deployed on Matic/Polygon which has an even higher gas limit still (although adjustable).
🌟 Selected for report: 0xRajeev
434.0526 USDC - $434.05
0xRajeev
The general definition of basis points is 100 bps = 1%. The usage here, 1000 bps = 100%, deviates from generally accepted definition and could cause confusion among users/creators/affiliates or potential miscalculations.
https://www.investopedia.com/terms/b/basispoint.asp
Manual Analysis
Document the used definition of basis points or switch to the generally accepted definition.
#0 - Splidge
2021-06-17T09:02:57Z
Yep, I discovered this also looking at one of the other issues. These have been changed to PER_MILLE which is equivalent to a MegaBip
#1 - Splidge
2021-06-21T10:45:58Z
Corrected alongside #10 in the commit here
🌟 Selected for report: 0xRajeev
434.0526 USDC - $434.05
0xRajeev
Factory constructor sets timeout to 86400 seconds but setter setTimeout() has no threshold checks for a min timeout value. If this is accidentally set to 0 or lower-than-safe value then there is no dispute window and users lose confidence in market.
Manual Analysis
Add input validation for threshold checks on both low and high ends.
#0 - Splidge
2021-06-17T09:32:09Z
The oracle doesn't allow a timeout of 0 so accidentally setting to 0 wouldn't cause market problems, a lower than safe value provides a warning to the users on the oracle. It is important that the users check the oracle anyway to get the specific wording of the question. In practice the owner and uberOwner will be a multisig and so a mistake would require multiple people to perform the same mistake.
🌟 Selected for report: 0xRajeev
434.0526 USDC - $434.05
0xRajeev
In the Factory contract, the Factory owner is authorized to change approval for governor addresses and is also treated as a governor in the modifier onlyGovernors. However, isGovernor modifier excludes Factory owner as a governor for some reason. This function is used only by Treasury to whitelist users who can deposit tokens and would make sense to include Factory owner as a governor to be consistent.
Without doing so, Factory owner cannot whitelist users without adding itself or someone else as a governor.
Manual Analysis
Include Factory owner in isGovernor().
#0 - Splidge
2021-06-17T10:11:56Z
While the owner has the ability to perform the tasks of a governor and the ability to make themselves a governor they might not actually be a governor so probably shouldn't be included in isGovernor as this could be used for other external reasons.
The addition of the owner into onlyGovernors is more of a convenience for the owner as they have the power to change this anyway, why make it harder for them?
#1 - dmvt
2021-07-09T16:05:41Z
I think this issue is a fair critique. The language should probably be cleaned up so future devs don't get confused as to when 'governors' include the factory owner and when not.
🌟 Selected for report: 0xRajeev
434.0526 USDC - $434.05
0xRajeev
Once market is approved and operational, changing approval to false should not be allowed or else it will prevent NFTs from being withdrawn to mainnet. All other Governor controlled variables are used during market creation and not thereafter, except this one. The other onlyGovernors functions only affect state before market creation but this one affects after creation.
Manual Analysis
Once market is approved and operational, changing approval to false should not be allowed.
#0 - Splidge
2021-06-21T10:50:35Z
The concept of trapping the NFTs has been removed and markets will now be created in the paused state.
marketPause has been redone such that governors may only remove marketPause provided that it hasn't been set by the owner.
This means once a governor approves a market it will be shown in the UI and unpaused, they may un-approve the market again however this will only hide it in the UI and does not pause it again.
Implemented here
🌟 Selected for report: 0xRajeev
434.0526 USDC - $434.05
0xRajeev
Collusion and sybil attacks are general problems with blockchain-based prediction markets and voting systems.
Collusion between market creator and bidders, where the creator creates a niche prediction market for which only they know the outcome with a higher degree of probability (than others) and either spawn fake users (sybils) to increase the pot size and lure victims to add bids. Creator or its fake users maintain the longest duration on the winning outcome (which they know with greater certainty than others) thus winning that market’s outcome and taking the victim's rents (winner-take-all-mode).
https://en.wikipedia.org/wiki/Sybil_attack
Manual Analysis
The general problem is hard to solve. Document and warn users suitably about risks involved.
#0 - Splidge
2021-06-18T10:10:44Z
This doesn't appear to be a problem with the code. There are warnings on the frontend. There is some quality control in the way markets are created by allowing governors to approve them and the question specifics must be clearly stated for the oracle. We have already had in a beta test the users disagree with the wording of an outcome and collectively invalidate the oracle thereby returning all rent paid to the users.
#1 - dmvt
2021-07-10T11:04:51Z
I'm going to let this one stand mostly to serve as an additional warning to users who take the time to read the audit report. I don't think there is any action to take beyond warnings on the frontend.
🌟 Selected for report: 0xRajeev
434.0526 USDC - $434.05
0xRajeev
Misplaced zero-address check for nfthub on L595 in createMarket() because nfthub cannot be 0 at this point as nfthub.addMarket() on L570 would have already reverted if that were the case.
Manual Analysis
Move nfthub zero-address check to before the call to nfthub.addMarket().
#0 - Splidge
2021-06-21T10:53:29Z
fixed here
🌟 Selected for report: 0xRajeev
434.0526 USDC - $434.05
0xRajeev
Missing _checkState(States.OPEN) on first line of rentAllCards() as specified on L617. These core market functions are supposed to operate only when market is open but the missing check allows control to proceed further in the control flow. In this case, the function proceeds to call newRental() which has a conditional check state == States.OPEN and silently returns success otherwise, without reverting.
Impact: rentAllCards does not fail if executed when market is closed or locked. newRental returns silently without failure when market is closed or locked.
Manual Analysis
Add a require() to check market open state in the beginning of all core market functions and revert with an informative error string otherwise.
#0 - Splidge
2021-06-21T11:05:36Z
I've added the check on rentAllCards() here.
I have not made newRental() revert because we use this to lock the market if the market is beyond it's locking time. If the market does get locked successfully then the UI will update to show this. If it doesn't get locked then the appropriate accounting hasn't been completed yet.
🌟 Selected for report: 0xRajeev
0xRajeev
updateLastRentalTime() function “tracks when the user last rented so they cannot rent and immediately withdraw thus bypassing minimum rental duration.”
This function currently always returns true and so there is no need to assert its return value, as done in newRental(), unless it was meant to return false in some scenarios which indicates missing constraint/logic. It is not clear what that might be.
Impact: Given that the minimum rental duration is one of the two key protection mechanisms, any missing logic/constraint here could affect the project significantly.
Manual Analysis
Validate constraint/logic to see if function should return false in any scenario. Remove assert at call site if otherwise.
#0 - Splidge
2021-06-18T10:28:21Z
Duplicate of #55
#1 - dmvt
2021-07-10T11:11:43Z
I do not see how this is a duplicate of #55.
#2 - Splidge
2021-07-12T10:39:44Z
Sorry, I think it must have been #53 I wanted to mark it a duplicate of. Although there is also some overlap with #83 as the assert wasn't used correctly.
🌟 Selected for report: 0xRajeev
434.0526 USDC - $434.05
0xRajeev
The exitedTimestamp flag is used to prevent front-running of user exiting and re-entering in the same block. The setting of this flag in exit() should really be inside the conditionals and triggered only if current owner or if bidExists. It currently assumes that either of the two will always be true which may not necessarily be the case.
Impact: A user accidentally exiting a card he doesn't own or have a bid for currently will be marked as exited and prevented from a newRental in the same block. User can prevent one's own newRental from succeeding, because it was accidentally triggered, by front-running it himself with an exit. There could be other more realistic scenarios.
Manual Analysis
Set exitedTimestamp flag only when the conditionals are true within exit()
#0 - Splidge
2021-06-21T11:17:35Z
fixed here
🌟 Selected for report: 0xRajeev
434.0526 USDC - $434.05
0xRajeev
The getBid() order book function is noted in its Natspec @dev comments as “@dev just to pass old tests, not needed otherwise @dev to be deleted once tests updated” but is left behind here.
This function could externally expose orderbook ordering (prev/next linked list) for malicious contracts to potentially time or price bids to its advantage.
Manual Analysis
Remove function as noted.
#0 - Splidge
2021-06-18T12:43:26Z
Remove function as noted.
It is to be removed one the tests are updated. However they are not yet, so it isn't to be removed yet.
It is simple enough for other tools to determine the order of the orderbook without this, the frontend manages this and displays the information to the users. This is not a vulnerability, but useful information for users to have at their disposal.
#1 - dmvt
2021-07-10T13:22:24Z
The issue as I see it is that this exposes orderbook order to other contracts, not just other tools. This opens up some potentially unwanted vectors if left in. IMO, you should have removed this prior to the contest or specifically commented that it should be ignored during the contest. Another potential approach would be to have a testing contract which adds this functionality for testing but does not get automatically deployed to production. If you forget about it and leave it in, it does represent a small risk.
117.1942 USDC - $117.19
0xRajeev
Function addToWhitelist actually implements toggleWhitelist (similar to many other such functions in this codebase) and not explicit addition/inclusion. If user is already whitelisted, this call will blacklist the user and vice-versa.
Impact: Users whitelisted will be blacklisted causing a DoS if this function is invoked twice on their address accidentally.
Scenario: Governor Alice whitelists Bob. Alice then receives a list of users who need to be whitelisted which includes Bob. Alice calls batchAddToWhitelist to whitelist everyone (without realising that the list includes already whitelisted Bob) but blacklists Bob as a result, causing a DoS for Bob who will not be allowed to deposit anymore.
Manual Analysis
Change function name to toggleWhitelist() so that callers are explicitly aware of the reversal of status. If not, consider adding a bool to specify the allowed status.
#0 - Splidge
2021-06-18T12:58:45Z
Duplicate of #49
#1 - dmvt
2021-07-10T14:42:40Z
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
#2 - dmvt
2021-07-11T10:51:18Z
duplicate of #49
0 USDC - $0.00
0xRajeev
The market state enum is: CLOSED, OPEN, LOCKED, WITHDRAW and so ranges from 0-3. Therefore, the _incrementState() function can only be called thrice and the assert() in it should be checking for '< 3' and not ‘<4’ because it will only be called thrice for changing state from 0-1, 1-2, 2-3, where the initial state to change from will be a maximum of 2 not 3.
Impact: Current check allows calling a fourth time when state is already 3 (WITHDRAW) and is incremented to 4 which is undefined in the enum. It is unclear if there are code paths that lead to this scenario.
Manual Analysis
Change assert(uint256(state) < 4); to assert(uint256(state) < 3);
#0 - Splidge
2021-06-18T12:31:42Z
Duplicate of #9
#1 - dmvt
2021-07-10T13:14:55Z
duplicate of #9
🌟 Selected for report: 0xRajeev
0 USDC - $0.00
0xRajeev
In Market sponsor() the call to treasury.checkSponsorship() checks allowance and balance of user. This is redundant because the call to treasury.sponsor downstream checks allowance again and insufficient balance would cause any transfer to fail anyway.
Impact: Given the gas sensitivity of the code base, removing this redundant check could help conserve gas and prevent any DoS from breaking gas limits.
Manual Analysis
Remove redundant checks.
#0 - Splidge
2021-06-18T12:21:57Z
conserve gas and prevent any DoS from breaking gas limits
Shouldn't this be Severity - G (Gas Optimisation)?
#1 - Splidge
2021-06-21T11:19:44Z
removed redundant check here
#2 - dmvt
2021-07-10T13:10:37Z
I agree... this is a gas optimization, not a risk issue