Altr

The `getValuation()` of the `PriceIndex` contract is used to valuate a given NFT. The function returns a `Valuation` struct, which contains the fields `timestamp`, `price` and `ltv`. However, that struct is missing crucial sanity checks regarding its values: **1. Missing check that `Valuation.ltv <= 100`** The actually borrowed amount needs to satisfy the following require-statement: ``` require(_amount <= (valuation.price * valuation.ltv) / 100, "Lending: amount greater than max borrow"); ``` If `valuation.ltv` reaches values higher than `100`, the resulting expression would be bigger than `valuation.price`. This would allow borrow amounts higher than the actual collateral value, where an unknowing lender may accept the loan and in the worst case remain with the overpaid NFT. **2. Missing check that the price data is sufficiently fresh** The `timestamp` field is never checked to be within a reasonable freshness threshold, which could lead to stale valuation prices and therefore to potentially gross over-/undervaluations and either costing the borrower or the lender unnecessary funds. **3. The `acceptLoan()` function is not double-checking the oracle** Currently, when accepting a loan, the loan request's collateral is not double-checked on-chain by the lender. Given that a loan request with a sufficiently high deadline could be arbitrarily old, the oracle should again be consulted to protect lenders from possibly accepting underwater loans.

In `Lending.sol#L61`, `L124`, `L204` and `L249` the code documentation states certain fees and rates to be in `basis points`. Basis points are usually defined as `10000 = 100%`, `1000 = 10%`, `100 = 1%`, `10 = 0.1%` and `1 = 0.01%`. However, in `Lending.sol#L22` the `PRECISION` is defined as: ``` uint256 public constant PRECISION = 10000; ``` and different protocol fee maxima are defined as multiple of that value: ``` uint256 public constant MAX_PROTOCOL_FEE = 4 * PRECISION; uint256 public constant MAX_REPAY_GRACE_FEE = 4 * PRECISION; uint256 public constant MAX_BASE_ORIGINATION_FEE = 3 * PRECISION; uint256 public constant MAX_LIQUIDATION_FEE = 15 * PRECISION; uint256 public constant MAX_INTEREST_RATE = 20 * PRECISION; ``` In the following lines values are wrongly computed: 1. `Lending.sol#L411`: `platformFee += (totalPayable * repayGraceFee) / PRECISION;` 1. `Lending.sol#L675`: `(_borrowedAmount * liquidationFee) / PRECISION` So the fees are not interpreted as percentage shares, but multiplying factors of the borrowed amount, resulting in the borrower being required to be pay a multiple of the borrowed amount as a penalty when paying during the grace period or liquidators being required to pay back a multiple of the borrowed amount as liquidation fees.

ALT-1 highlighted that the current test suite is inadequately testing the outputs of functions from the smart contract. With closer inspection, rather than testing the functional correctness, most of the tests seem to have been built with the smart contract output in mind, as e.g. they simply compare the smart contract result with hardcoded values. The test `testGracePeriod()` in `Lending.t.sol` for example can be seen to consider a loan repayment within the grace period with over 300% interest rate as valid, as the `repayGraceFee` is set to `25000`, being interpreted as 250%. Overall, we deemed the test suite to be slightly minimalistic around some of the features of the protocol too, such as multiple possible loan durations and the application of the fee reduction factor.

While the protocol allows for multiple different borrow tokens to be used, as defined with `setTokens()`, the value returned by the `PriceIndex` valuation contract when requesting a new loan is indifferent to the target borrow token and its decimals, always returning the same nomination. Given an NFT valuation, the following check would be indifferent to the target borrow token decimals, potentially grossly mistreating an allowed borrow amount: ``` require(_amount <= (valuation.price * valuation.ltv) / 100, "Lending: amount greater than max borrow"); ``` For example, while the valuation could be designed for tokens with 6 decimals, requesting a loan for a token with more decimals would lead to unnecessary borrow restrictions as a potentially only much lower amount would pass the check. Similarly, a token with less than 6 decimals may allow for borrow amounts exceeding the actual collateral value.

Certain contracts have state variables, e.g. `owner`, which provide certain addresses with privileged roles. Such roles may pose a risk to end-users. The `owner` address has sole permission to call the following functions: * `setPriceIndex()` to update the pricing oracle. Malicious oracles could return incorrect values at the disadvantage of lenders. * `setGovernanceTreasury()` to update the address the protocol fees are transferred to. * `setRepayGracePeriod()` to update the period under which users can repay their expired loan before being liquidated. * `setRepayGraceFee()` to update the fee associated with such a delayed repayment. * `setProtocolFee()` to update the fee applied to the borrowed amount. * `setLiquidationFee()` to update the fee applied to the borrowed amount. * `setBaseOriginationFee()` to update the fee regarding the origination fees of the protocol. * `setTokens()` to add token addresses to be requestable for loans. * `unsetTokens()` to remove token addresses from that list * `setLoanTypes()` to update or add the APRs for certain loan durations. * `unsetLoanTypes()` to remove certain loan durations from being requestable for loans. * `setFeeReductionFactor()` to update the factor by which to reduce the origination fee given the loan exceeds a range. * `setRanges()` to update the origination fee ranges determining how often the `feeReductionFactor` is applied. While the core interest rate fetched via the aprFromDuration mapping updateable via the `setLoanTypes()` mapping is hard coded at the time the loan is requested, it should be highlighted that in following fee calculations of the loan, the other fees are always fetched from the latest state of the contract, possibly differing from the fees the borrower originally requested the loan for. So technically, it is possible that accepted loans at the time of repayment face a different set of `originationBaseFee`, `gracePeriodFee`, and `protocolFee`, though reasonable constant maximum values for those fees are enforced, enabling them only to collectively reach a value of 11 %. Also, loans that are within the grace period could become instantly liquidatable if the owner would e.g. suddenly greatly reduce the `gracePeriod`.

Currently, the oracle used to assess the collateral value of an NFT is only consulted once when the loan is originally requested. The protocol has been designed in a way that loans can not be liquidated once the collateral falls below a certain value; a loan can only ever be liquidated if the loan has not been repaid within the loan's expiry date with an added grace period. When the value of the NFT collateral falls sufficiently the loan becomes undercollateralized, which indicates that the borrower lacks sufficient collateral, thereby exposing the lender to the risk of potential loss. Additionally, in case of the loan becoming undercollateralized, the incentives for liquidators diminish too: Liquidators pay off the original loan and its accumulated interest rate and pay an additional liquidation fee to the protocol treasury, for which they get the NFT collateral in return. If the loan were to be undercollateralized at this point, liquidators would suffer a net loss, thereby would never invoke a liquidation. So the only possible course of action for lenders would be to claim the NFT that is now worth less than the due loan and interest. While this lender-side risk is present in other peer-to-peer NFT lending protocols, we noticed that it is not mentioned in the provided documentation.

In order to validate the proper deployment and initialization of the contracts, it is a good practice to emit events. Also, any important state transitions can be logged, which is beneficial for monitoring the contract, and also tracking eventual bugs or hacks. Below we present a non-exhaustive list of events that could be emitted to improve application management: * `setRanges()` should emit an `OriginationFeesRangesSet()` event * `setFeeReductionFactor()` should emit a `FeeReductionFactorSet()` event

The owner of the contracts can call `transferOwnership()` to transfer the ownership to a new address. If an uncontrollable address is accidentally provided as the new owner address then the contract will no longer have an active owner, and functions with the `onlyOwner` modifier can no longer be executed.

Currently, the contract does not make use of explicit rounding. Rounding should always be performed in a way that reduces potential attack vectors around abusing Solidity's implicit rounding. Therefore, the return value of the `getDebtWithPenalty()` function should be rounded upwards before being converted to a `uint256` again.

In the `getDebtWithPenalty()` function, only at the very end of the calculation the calculation of the `accruedDebt` variable, the value is converted to a `UD60x18`. In extreme edge cases this can lead to integer truncation where interest rate will be zero. Granted, we were only able to come up with zero interest rate cases for loans of unrealistically small amounts, yet we still think it is worth addressing.