Deposit double-counting miscalculation could incorrectly prevent user foreclosure

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

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.

Proof of Concept

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.

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCTreasury.sol#L279

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCTreasury.sol#L303

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCTreasury.sol#L308-L314

Tools Used

Manual Analysis

Recommended Mitigation Steps

Change the conditional predicate on L309-310 from:
user[_user].deposit + _amount > user[_user].bidRate / minRentalDayDivisor
to:
user[_user].deposit > user[_user].bidRate / minRentalDayDivisor

[Splidge]

Duplicate of #26

[dmvt]

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.