minRentalDayDivisor can be different between markets and treasury

[code423n4]

Handle

gpersoon

Vulnerability details

Impact

The minRentalDayDivisor is defined in RCTreasury.sol and copied to each market.
The minRentalDayDivisor can be updated via setMinRental, but then it isn't updated in the already created market.

To calculate the minimum rent time, in function withdrawDeposit of RCTreasury.sol, the latest version of minRentalDayDivisor is used, which could be different than the values in the market.
So the markets will calculate the minimum rent time different.
This could lead to unexpected results

Proof of Concept

// https://github.com/code-423n4/2021-06-realitycards/blob/main/contracts/RCMarket.sol#L191
function initialize(
...
minRentalDayDivisor = treasury.minRentalDayDivisor();

https://github.com/code-423n4/2021-06-realitycards/blob/main/contracts/RCTreasury.sol#L322
function withdrawDeposit(uint256 _amount, bool _localWithdrawal)
...
require( user[_msgSender].bidRate == 0 || block.timestamp - (user[_msgSender].lastRentalTime) > uint256(1 days) / minRentalDayDivisor, "Too soon");
..
if ( user[_msgSender].bidRate != 0 && user[_msgSender].bidRate / (minRentalDayDivisor) > user[_msgSender].deposit ) {
..

// https://github.com/code-423n4/2021-06-realitycards/blob/main/contracts/RCTreasury.sol#L169
function setMinRental(uint256 _newDivisor) public override onlyOwner {
minRentalDayDivisor = _newDivisor;
}

Tools Used

Recommended Mitigation Steps

Either accept and document the risk or change to code to prevent this from happening.

[Splidge]

Yes, This became apparent recently when we changed the minRentalDayDivisor during a beta test.
Ideally this value is never changed and if it is changed then it will be done very infrequently.
The main protection minRentalDayDivisor offers is against a DoS attack whereby an attacker gains some ownership time on a card and then will fill the orderbook with bids using sybil accounts (withdrawing almost all deposit after placing the bids), without minRentalDayDivisor these low value (but legitimate) bids would prevent other users from gaining ownership of the card (due to gas limits there's a limit to the rental collections we can perform) and give the attacker a greater share of the prize pot. The benefit of minRentalDayDivisor is that now these are zero value bids which are eligible for immediate deletion, and so there is now more of a cost to the attack which scales with the cost of the rental prices (which will closely be linked to the value of the prize pot). To this end minRentalDayDivisor is at it's most useful in the Treasury where it's main purpose is fulfilled in withdrawDeposit(), the usage in the markets is less beneficial and wasn't considered worth the extra gas usage to have the Markets fetch the updated value given the infrequency we will be changing it.
We have accepted this risk.

[dmvt]

Updating to Medium risk to match the other reporting wardens: "Possible accidental loss of funds or information due to code manipulation or bad side effects of not properly outlining a payable function"