AbraNFT contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L287-L288 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/interfaces/INFTOracle.sol#L5-L10

Vulnerability details

Impact

The INFTOracle.get() function returns two values. A boolean specifying whether a valid rate is available and a uint specifying the current price: https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/interfaces/INFTOracle.sol#L5-L10

The NFTPairWithOracle contract only checks the second value and ignores the first one. If the oracle doesn't have a valid price it will return (false, 0) since the zero-value of the uint will probably be used in a such a case.

If that happens, the lender is able to seize the borrower's collateral. Just because the oracle doesn't have a recent value doesn't mean that the collateral's value dropped. In such a case, the lender shouldn't be able to seize it. Instead, it should simply be ignored until the oracle is able to provide a valid value.

Proof of Concept

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

uint256 amount = loanParams.valuation + interest;
(, uint256 rate) = loanParams.oracle.get(address(this), tokenId);
require(rate.mul(loanParams.ltvBPS) / BPS < amount, "NFT is still valued");

If the oracle returns (false, 0) the require statement will always be true since the left side of the inequality will always be 0 while the right side will always be larger than 0.

Tools Used

none

Recommended Mitigation Steps

Change the linked lines to:

uint256 amount = loanParams.valuation + interest;
(bool success, uint256 rate) = loanParams.oracle.get(address(this), tokenId);
require(success, "oracle doesn't return valid value");
require(rate.mul(loanParams.ltvBPS) / BPS < amount, "NFT is still valued");

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L198-L223 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L200-L212 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L288

Vulnerability details

Impact

The lender should only be able to seize the collateral if:

• the borrower didn't repay in time
• the collateral loses too much of its value

But, the lender is able to seize the collateral at any time by modifying the loan parameters.

Proof of Concept

The updateLoanParams() allows the lender to modify the parameters of an active loan in favor of the borrower. But, by setting the ltvBPS value to 0 they are able to seize the collateral.

If ltvBPS is 0 the following require statement in removeCollateral() will always be true:

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

rate * 0 / BPS < amount is always true.

That allows the lender to seize the collateral although its value didn't decrease nor did the time to repay the loan come.

So the required steps are:

1. lend the funds to the borrower
2. call updateLoanParams() to set the ltvBPS value to 0
3. call removeCollateral() to steal the collateral from the contract

Tools Used

none

Recommended Mitigation Steps

Don't allow updateLoanParams() to change the ltvBPS value.

Report

Pair can be initialized without valid asset

In NFTPair.init() you verify that the collateral is not a zero-address. But, you don't do the same thing for the asset. If either of them is not a valid address the NFTPair is unusable. So if you do it for the collateral you should also do it for the asset.

Relevant code:

• https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L175-L179

Add:

require(address(asset) != address(0), "NFTPair: invalid asset");

Use ERC721 safe methods when transferring to an arbitrary address

The ERC721 safe methods check whether the receiving address is able to handle ERC721 tokens. In removeCollateral() you send the collateral to a user defined address. If the user makes a mistake and uses a contract that can't handle the collateral the token will be locked up.

You might argue that it's the user's fault and not the contract's responsibility. But switching to the safe method wouldn't introduce any reentrancy risk. The collateral is already transferred at the very end of the function. After all the relevant state variables are updated.

It provides a little more protection for the user with a small increase in gas costs.

Relevant code:

• https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L272-L273