c4-bot-2 commented 10 months ago

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L159-L179 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L132-L146 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L100-L118

Vulnerability details

Impact

The PaymentEscrow contract is responsible for escrowing rental payments while rentals are active. When rentals are stopped, this module determines payouts to all parties. For PAY orders that are stopped before the rental period has ended, the _settlePaymentProRata() function in this contract calculates the pro-rata payment for the renter and lender and transfers the respective amounts.

However, there is a potential issue with the implementation of this function. It first calls the internal function _calculatePaymentProRata() to calculate the renter and lender amounts. Due to the precision loss in the calculation, there can be scenarios where either the renterAmount or the lenderAmount becomes zero, both close to the beginning and the end of the total duration of the rental.

The problem arises when these zero amounts are passed on to the _safeTransfer()function. Some ERC20 tokens revert when attempting to transfer 0 tokens. If such a token is used as payment in a PAY order, the transaction will revert at certain periods, preventing the renter or lender from stopping the rental. In an (unrealistic) worst-case scenario, this locks the funds until the rental period is over.

Proof of Concept

Let's consider the extreme case of a PAY order with amount = 1 and totalTime = 1000. The calculation of renterAmount in the original code is as follows:

$$ \begin{aligned} \text{numerator} &= (\text{amount} \times \text{elapsedTime}) \times 1000, \ \text{renterAmount} &= \left(\frac{\text{numerator}}{\text{totalTime}} + 500\right) / 1000. \end{aligned} $$

Substituting amount = 1 and totalTime = 1000, we get:

$$ \begin{aligned} \text{numerator} &= (1 \times \text{elapsedTime}) \times 1000, \ \text{renterAmount} &= \left(\frac{\text{elapsedTime} \times 1000}{1000} + 500\right) / 1000 \ &= \frac{\text{elapsedTime} + 500}{1000}. \end{aligned} $$

For elapsedTime < 500, the renterAmount becomes less than 1, as the addition of 500 and division by 1000 results in a value less than 1. Since renterAmount must be an integer, it will be rounded down to 0.

Conversely, for elapsedTime ≥ 500, renterAmount will be 1, as the sum of elapsedTime and 500 will be 1000 or more, making the division result 1. This leaves no value for lenderAmount, effectively making it 0.

To summarize, for elapsedTime < 500, renterAmount will be zero, and for elapsedTime >= 500, lenderAmount will be 0. If the ERC20 token used in this PAY order reverts on zero value transfers, it would not be possible to stop the rental at any point until it is over.

While this is an extreme scenario, the issue does also arise for less extremes values of amount and totalTime and leads to periods during which an order cannot be stopped.

Tools Used

Manual review

Recommended Mitigation Steps

A simple fix would be to add a check for zero value amounts in the _safeTransfer() function and skip the transfer if the amount is zero. This would prevent the transaction from reverting and allow the rental to be stopped as expected.