Outstanding loan Oracle can be arbitrary changed by a lender

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L205-L211

Vulnerability details

Impact

As updateLoanParams() function do not verify params.oracle, a lender for an already outstanding loan can change params.oracle to a non-market one.

For example, the lender can set oracle to a pre-cooked INFTOracle contract reporting 0 price of the asset and immediately seize it with removeCollateral().

Setting the severity to high as that's a violation of the logic of the system and asset loss for the borrower.

Proof of Concept

updateLoanParams do not require that params.oracle remains the same:

        if (loan.status == LOAN_OUTSTANDING) {
            // The lender can change terms so long as the changes are strictly
            // the same or better for the borrower:
            require(msg.sender == loan.lender, "NFTPair: not the lender");
            TokenLoanParams memory cur = tokenLoanParams[tokenId];
            require(
                params.duration >= cur.duration &&
                    params.valuation <= cur.valuation &&
                    params.annualInterestBPS <= cur.annualInterestBPS &&
                    params.ltvBPS <= cur.ltvBPS,
                "NFTPair: worse params"
            );

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L205-L211

In the same time removeCollateral allows for collateral retrieval anytime as long as Oracle reports low enough price:

            if (uint256(loan.startTime) + tokenLoanParams[tokenId].duration > block.timestamp) {
                TokenLoanParams memory loanParams = tokenLoanParams[tokenId];
                // No underflow: loan.startTime is only ever set to a block timestamp
                // Cast is safe: if this overflows, then all loans have expired anyway
                uint256 interest = calculateInterest(
                    loanParams.valuation,
                    uint64(block.timestamp - loan.startTime),
                    loanParams.annualInterestBPS
                ).to128();
                uint256 amount = loanParams.valuation + interest;
                (, uint256 rate) = loanParams.oracle.get(address(this), tokenId);
                require(rate.mul(loanParams.ltvBPS) / BPS < amount, "NFT is still valued");
            }

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L277-L289

This way the lender can change the oracle and seize the asset anytime.

Recommended Mitigation Steps

The most straightforward solution is to add params.oracle to the verification list in updateLoanParams() for LOAN_OUTSTANDING state:

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L205-L206

require(
    params.oracle == cur.oracle &&
    params.duration >= cur.duration && ...

[cryptolyndon]

Duplicate of #37