AbraNFT contest - 0x1337's results

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L286-L288 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L200-L211

Vulnerability details

Impact

The intended use of the Oracle is to protect the lender from a drop in the borrower's collateral value. If the collateral value goes up significantly and higher than borrowed amount + interest, the lender should not be able to seize the collateral at the expense of the borrower. However, in the NFTPairWithOracle contract, the lender could change the Oracle once a loan is outstanding, and therefore seize the collateral at the expense of the borrower, if the actual value of the collateral has increased significantly. This is a critical risk because borrowers asset could be lost to malicious lenders.

Proof of Concept

In NFTPairWithOracle, the params are set by the borrower when they call requestLoan(), including the Oracle used. Once a lender agrees with the parameters and calls the lend() function, the loan.status changes to LOAN_OUTSTANDING.

Then, the lender can call the updateLoanParams() function and pass in its own params including the Oracle used. The require statement from line 205 to 211 does not check if params.oracle and cur.oracle are the same. A malicious lender could pass in his own oracle after the loan becomes outstanding, and the change would be reflected in line 221.

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

In a situation where the actual value of the collateral has gone up by a lot, exceeding the amount the lender is owed (principal + interest), the lender would have an incentive to seize the collateral. If the Oracle is not tampered with, lender should not be able to do this, because line 288 should fail. But a lender could freely change Oracle once the loan is outstanding, then a tampered Oracle could produce a very low rate in line 287 such that line 288 would pass, allowing the lender to seize the collateral, hurting the borrower.

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

Tools Used

Manual review

Recommended Mitigation Steps

Once a loan is agreed to, the oracle used should not change. I'd recommend adding a check in the require statement in line 205 - 211 that params.oracle == cur.oracle

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L177 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L194 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L330-L337 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L295-L302 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L591-L598

Vulnerability details

Impact

In the NFTPair and NFTPairWithOracle contracts and their clones, there's no check in the init() function or elsewhere that the asset is not deflationary. A deflationary token, such as any Safemoon fork, charges a fee for each transfer, and as such when address A sends amount to address B, address B's balance increases by less than amount due to such fee.

The BentoBox contract is not compatible with deflationary token, given how user balance are recorded in the deposit, withdraw, and transfer functions. Therefore, the risk of allowing deflationary token in NFTPair and NFTPairWithOracle is that the BentoBox contract might suffer a loss in asset as lending and repayment continue. And the lender might be unable to withdraw its balance from BentoBox even if the borrower repays the principal plus interest, as the BentoBox might run out of asset balance for the lender to withdraw. This causes loss of user fund, and could therefore be a major problem.

Proof of Concept

You could see in the init() function that asset can be whatever that the caller of this contract provides.

    function init(bytes calldata data) public payable override {
        require(address(collateral) == address(0), "NFTPair: already initialized");
        (collateral, asset) = abi.decode(data, (IERC721, IERC20));
        require(address(collateral) != address(0), "NFTPair: bad pair");
    }

When lending or repayment or withdraw happens, asset transfer or withdraw is handled by bentoBox, where there's no accounting for any potential transfer fee. bentoBox contract might therefore run out of asset balance, causing users unable to withdraw the full amount via the function below.

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

Tools Used

Manual Review

Recommended Mitigation Steps

I'd recommend disallowing deflationary token in these contracts. There are different ways to do this. One possible way is to ask a user to send certain amount of token to the contract, and then check if the increase in the contract's token balance equals the amount being sent. The asset is not deflationary if the check passes. Then a global variable passed could be used to indicate that. Only then could lending be allowed.

    function checkAsset(uint256 amount) public {
        uint256 oldBalance = asset.balanceOf(address(this));
        require(asset.transferFrom(msg.sender, address(this), amount), "transferFrom failed");
        uint256 newBalance = asset.balanceOf(address(this));
        require(newBalance == oldBalance + amount, "no deflationary token allowed");
        require(asset.transfer(msg.sender, amount), "transfer failed");
        passed = true;
    }