findNewOwner edgecase

[code423n4]

Handle

gpersoon

Vulnerability details

Impact

In the function findNewOwner of RCOrderbook, as loop is done which included the check _loopCounter < maxDeletions
Afterwards a check is done for "(_loopCounter != maxDeletions)" to determine if the processing is finished.
If _loopCounter == maxDeletions then the conclusion is that it isn't finished yet.

However there is the edgecase that the processing might just be finished at the same time as _loopCounter == maxDeletions.

You can see this the best if you assume maxDeletions==1, in that case it will never draw the conclusion it is finished.
Of course having maxDeletions==1 is very unlikely in practice.

Proof of Concept

// https://github.com/code-423n4/2021-08-realitycards/blob/main/contracts/RCOrderbook.sol#L549
function findNewOwner(uint256 _card, uint256 _timeOwnershipChanged) external override onlyMarkets {
...
// delete current owner
do {
_newPrice = _removeBidFromOrderbookIgnoreOwner( _head.next, _market, _card );
_loopCounter++; // delete next bid if foreclosed
} while ( treasury.foreclosureTimeUser( _head.next, _newPrice, _timeOwnershipChanged ) < minimumTimeToOwnTo &&
_loopCounter < maxDeletions );

    if (_loopCounter != maxDeletions) {   // the old owner is dead, long live the new owner
        _newOwner = .... 
        ...
    } else {
        // we hit the limit, save the old owner, we'll try again next time
       ...
    }
}

Tools Used

Recommended Mitigation Steps

Use a different way to determine that the processing is done. This could save some gas.
Note: the additional check also costs gas, so you have the verify the end result.

Perhaps in setDeletionLimit doublecheck that _deletionLimit > 1.

[Splidge]

oh wow, this is actually a really big problem. It's easier to see it if maxDeletions is 1 but it exists with any size of maxDeletions.
Whenever we find a valid owner on the final iteration of the loop the if statement will simply check if it was the final loop. That valid owner is then assumed to be invalid and saved for the next transaction to try and find a new owner. When that next transaction happens the valid owner is immediately deleted and not given any ownership of the card at all.
I think this just falls short of 3 (High risk) because I don't think it'd be possible for an attacker to engineer the situation to have a particular user deleted without ownership. But I believe this would count as 2 (Med risk) because the protocol "availability could be impacted" for the user that is deleted.

[Splidge]

I have since thought of an attack that could have used this and might raise it to 3 (High risk).

Due to the difficultly of monitoring which cards you own all the time a valid strategy which some users employ is to bid high enough to scare off other users (usually bidding significantly beyond the 10% minimum increase). Suppose Alice employs this strategy by bidding $100 on a card that was previously only $10.
Mal (our attacker) wishes to rent the card but wants to pay less than $100. Mal could use Sybil accounts to place maxDeletions - 1 bids all for the minimum rental duration (only funding the accounts for the minimum duration). Mal would then need to wait for the minimum duration of all these bids to expire, (maxDeletions - 1 ) * minimumRentalDuration
Once this has completed Mal can place a bid at $11, this will trigger a rent collection which will attempt to findNewOwner, Alice being the user that was found on the last iteration of the loop would be considered as invalid. There will not be a change of ownership or any events emitted about this until the next rent collection is triggered.
This means that the UI would still consider Alice to be the owner of card (Mals' Sybil bids having had LogRemoveFromOrderbook and LogUserForeclosed events emitted) and other users might not consider trying to outbid this, whereas actually Mal is accruing time at a significantly cheaper rate.

Thinking about it, this doesn't really even need Alice at all, Mal could have placed all the higher bids to simultaneously scare off other users while renting at a lower price.

I think the fix is relatively simple, by checking if we found a valid user OR hit the deletion limit we can make it so that we don't skip any bids. This would then leave Alice (or Mal in the other version) correctly having to pay for the time at the higher price.

[0xean]

upgrading based on sponsors analysis

[Splidge]

Fixed here