AbraNFT contest - kenzo's results

Lines of code

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

Vulnerability details

When requesting a loan, the borrower sets the used oracle. Afterwards the lender can change the loan parameters as long as they are favorable to the borrower. However, the contract is not checking whether the oracle has been updated. This means the lender can immediately change a loan's oracle to be an oracle of his choosing.

Impact

The lender can set the loan's oracle to be an oracle that puts the loan underwater. Therefore the borrower's collateral can be liquidated and taken unjustly.

Proof of Concept

The TokenLoanParams struct contains the oracle to be used, amongst other fields:

struct TokenLoanParams {
    uint128 valuation; // How much will you get? OK to owe until expiration.
    uint64 duration; // Length of loan in seconds
    uint16 annualInterestBPS; // Variable cost of taking out the loan
    uint16 ltvBPS; // Required to avoid liquidation
    INFTOracle oracle; // oracle used
}

When the lender updates loan parameters in updateLoanParameters, the new oracle is not checked:

            require(
                params.duration >= cur.duration &&
                    params.valuation <= cur.valuation &&
                    params.annualInterestBPS <= cur.annualInterestBPS &&
                    params.ltvBPS <= cur.ltvBPS,
                "NFTPair: worse params"
            );

The loan parameters are then updated as a whole:

        tokenLoanParams[tokenId] = params;

Therefore, the oracle will be also updated, and the lender can choose an oracle that will put the loan underwater and then liquidate the collateral immediately. (Note: also, the log emitted does not log the new oracle.)

Recommended Mitigation Steps

Either don't allow the lender to change the oracle, or allow it only if the borrower agrees to the new oracle.

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L218 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L238

Vulnerability details

_requestLoan makes an external call to the collateral contract before updating the NFTPair contract state.

Impact

If the ERC721 collateral has a afterTokenTransfer hook, The NFTPair contract can be reentered, and a loan can be requested without the borrower supplying the collateral. Someone can then lend for the loan while the collateral is missing from the contract. Therefore the malicious borrower received the loan without supplying collateral - so lender's funds can be stolen. The issue is present in both NFTPair and NFTPairWithOracle.

Proof of Concept

Assume the NFT contract has an afterTokenTransfer hook which calls back to the malicious borrower. POC in short: borrower will call requestLoan with skim==false, then during the hook will reenter requestLoan with skim==true, then call removeCollateral to get his collateral back, then the first requestLoan will resume and initiate the loan, although the collateral is not in the contract any more.

POC in long: the borrower will do the following:

• Call requestLoan with skim==false.
• requestLoan will call collateral.transferFrom().
• The collateral will be transferred to the NFTPair. Afterwards, the ERC721 contract will execute the afterTokenTransfer hook, and hand control back to Malbo (malicious borrower).
• Malbo will call requestLoan again with skim==true.
• As the first request's details have not been updated yet, the tokenId status is still LOAN_INITIAL, and the require statement of the loan status will pass.
• The NFT has already been transfered to the contract, so the require statement of token ownership will pass.
• requestLoan (the second) will continue and set the loan details and status.
• After it finishes, still within the afterTokenTransfer hook, Malbo will call removeCollateral. His call will succeed as the loan is in status requested. So the collateral will get sent back to Malbo.
• Now the afterTokenTransfer hook finishes.
• The original requestLoan will resume operation at the point where all the loan details will be updated.
• Therefore, the contract will mark the loan is valid, although the collateral is not in the contract anymore. A lender might then lend funds against the loan without Malbo needing to pay back.

Recommended Mitigation Steps

Move the external call to the end of the function to conform with checks-effects-interaction pattern.

Attacker may call collateral/BentoBox before they are initialized

The _call function checks that the external call is not to BentoBox/collateral contracts, as then an attacker could call them and approve himself to spend NFTPair's funds:

require(callee != address(bentoBox) && callee != address(collateral) && callee != address(this), "NFTPair: can't call");

These address variables are being set in the init function. If an attacker calls callbefore the init function has been called, he may execute the aforementioned malicious code. Now, I realize this will probably not happen with AbraNFT, as you are deploying the NFTPair using Bento's deploy, which initializes the NFTPair atomically. So this can not happen. However, the risk is there in case somebody forks your project and changes the flow, as we've seen for example in the various Compund hacks and it's tech debts. So while you are basically not as risk, I suggest you mediate this vector for the potential good of humankind. You can do so by requiring that collateral != 0 etc' or by adding a comment notifying about the risk. May all human beings be happy.

Lend function not updating loan parameters?

The lend function takes as an argument from the lender the loan params, and checking that they are not worse for the borrower. It then uses the parameters set by the borrower to initialite the loan, and not these parameters from the lender. I think it is a design choice as to which version to use, but just wanted to point this out in case this is by mistake.