AbraNFT contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L312-L318 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L212-L215

Vulnerability details

Impact

As neither lend(), nor updateLoanParams() functions verify params.oracle, the lend call can be front run by a malicious borrower with changing params.oracle to a non-market one.

The front run will be an updateLoanParams call where borrower introduces fake oracle that will post non-market constant or ever increasing NFT price. This will render void collateral protection as borrower will have it manipulated.

Setting the severity to high as that's a clear violation of the stated logic of the system, which can lead to lender's losses, that were protected by oracle reporting otherwise.

Proof of Concept

lend() -> lend() call used to accept the loan do not verify params.oracle:

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

updateLoanParams() do allow to change any loan parameters in the LOAN_REQUESTED state:

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

This way, when lender are to accept the terms the borrower can switch params.oracle to any pre-cooked contract reporting any manipulated non-market prices, so that there effectively will be no mark-to-market collaterization and borrower can go on with not enough collaterized loan, which isn't what lender will expect, for example accepting a lower interest rate because asset price is reported and liquidation in a bad scenario is possible.

Recommended Mitigation Steps

As the deals are OTC, the lender has to verify Oracle manually and in order to avoid the front running the simplest solution is to add params.oracle to the verification list in _lend():

require( params.oracle == accepted.oracle && 
	params.valuation == accepted.valuation &&
		...

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

Another solution is to forbid changing oracle in updateLoanParams, but it looks to be overly restrictive.

Notice that best practice here is an introduction of oracle white list, so only verified oracles can be used for price discovery. Namely, given that collateral NFT contract is fixed within each NFTPairWithOracle, it is advised to have a short list of verified oracles for that NFT and allow nothing else.

Lines of code

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

Vulnerability details

Impact

The system can use stale or even plainly incorrect (due to any technical malfunction) price for decision making.

For example, a malicious lender can setup a bot that tracks incorrect readings (i.e. track the state of the Oracle used and act on observing (false, wrong_price) reply whenever wrong_price is lucrative enough), and end up seizing the collateral with removeCollateral(), while the position is healthy.

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

Proof of Concept

INFTOracle have success state and rate returned in get():

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/interfaces/INFTOracle.sol#L8

However, the state is ignored in NFTPairWithOracle both usages, the call is (, uint256 rate) = loanParams.oracle.get(address(this), tokenId):

In removeCollateral:

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

And in _lend:

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

As lend can be in borrower favor only, while is called by a lender, there looks to be no malicious usage.

However, removeCollateral is called by a lender and there Oracle malfunction is in her favor, so it is economically viable to her to act on wrong Oracle reading that is passed to the system as a result of ignoring Oracle state.

Notice that it is not Oracle malfunction, but improper Oracle usage situation.

Recommended Mitigation Steps

Consider requiring get() success in order to use the Oracle reading:

(bool res, uint256 rate) = loanParams.oracle.get(address(this), tokenId);
require(res, "Non-valid price");

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

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 && ...

Lines of code

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

Vulnerability details

Impact

Lender can accept overly restrictive LTV (the lowest possible at the moment), with high enough probability being able to seize the collateral after a short time.

Lender can set ltvBPS to zero with and immediately liquidate with removeCollateral any loan no matter how healthy it is.

Proof of Concept

In updateLoanParams a lender can set lower LTV for an already outstanding loan than borrowed initiated:

params.ltvBPS <= cur.ltvBPS,

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

In _lend a lender can accept lower LTV than borrowed anticipates:

params.ltvBPS >= accepted.ltvBPS,

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

In fact she can accept boundary LTV, equal to (params.valuation * BPS) / rate, so that the opening condition is met with equality:

require(rate.mul(uint256(params.ltvBPS)) / BPS >= params.valuation, "Oracle: price too low.");

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

In the same time TVL has 'an accepted part of collateral valuation to cover the total obligations' meaning (i.e. 1 - haircut), so the bigger part accepted the better, the bigger LTV parameter the better.

Say if LTV parameter is zero no value from collateral is accepted to cover the loan, so it is always underwater and liquidation condition is always true:

require(rate.mul(loanParams.ltvBPS) / BPS < amount, "NFT is still valued");

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

That is, a lender can updateLoanParams for any healthy loan setting ltvBPS to zero and immediately liquidating it with removeCollateral.

Recommended Mitigation Steps

Inverse the requirements for params.ltvBPS: the bigger the better.

1. ecrecover will return zero for non-matching signature (low)

Impact

Signature checks in takeCollateralAndLend and requestAndBorrow in NFTPair/NFTPairWithOracle can be passed with ecrecover returning 0x0, executing _requestLoan and _lend with zero borrower and lender correspondingly. It will lead to a low level error on transfer from zero address (given that skim is false) in both cases.

However, the ability to enter _requestLoan and _lend with unintended parameters should't exist and error reporting should generally be as transparent as possible, so relying on low level exceptions aren't advised.

As that's best practice with a small chance of unveiling of yet unseen attack surface (for example, can BentoBox be manipulated to have non-zero 0x0 balance?), setting the severity to be low.

Proof of Concept

takeCollateralAndLend doesn't check the ecrecover result to be meaningful:

  require(ecrecover(_getDigest(dataHash), v, r, s) == borrower, "NFTPair: signature invalid");
  _requestLoan(borrower, tokenId, params, borrower, false);
  _lend(msg.sender, tokenId, params, skimFunds);

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

Same for requestAndBorrow:

            require(ecrecover(_getDigest(dataHash), v, r, s) == lender, "NFTPair: signature invalid");
        }
        _requestLoan(msg.sender, tokenId, params, recipient, skimCollateral);
        _lend(lender, tokenId, params, false);

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

In the same time, there is a way to have ecrecover deterministically return zero (which means the function didn't found any answer, so returning the initial empty value):

https://ethereum.stackexchange.com/questions/69328/how-to-get-the-zero-address-from-ecrecover

Recommended Mitigation Steps

Consider adding to require(<user> != address(0), "Address cannot be zero") to takeCollateralAndLend and requestAndBorrow in NFTPair and NFTPairWithOracle (<user> is borrower and lender correspondingly).

This will also unify the approach with BentoBox:

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/BentoBoxFlat.sol#L561-L561

2. It is possible to reenter requestLoan doubling event emitted (low)

Impact

Event will be issued twice, one with arbitrary incorrect parameters supplied by a malicious borrower, the another with correct set of parameters subsequently used.

This is a griefing case as there looks to be no immediate economic benefit for an attacker, however system behaviour is altered in a wrong way.

As events are used for system analysis and some programmatic execution can be based on them, that looks to be a low severity issue.

Proof of Concept

requestLoan can be reentered:

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

_requestLoan can be called twice, first time with skim == false, then with skim == true:

  require(tokenLoan[tokenId].status == LOAN_INITIAL, "NFTPair: loan exists");
  if (skim) {
      require(collateral.ownerOf(tokenId) == address(this), "NFTPair: skim failed");
  } else {
      collateral.transferFrom(collateralProvider, address(this), tokenId);
  }
  TokenLoan memory loan;
  loan.borrower = to;
  loan.status = LOAN_REQUESTED;

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

The second call can be hooked by collateral.transferFrom as ERC721 transfer do have callbacks, for example:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L339-L350

The outcome to be double event emission, with first time event having arbitrary incorrect parameters.

Recommended Mitigation Steps

Consider adding nonReentrant modifier to requestLoan (and other skim using user-facing methods).

3. Different lender borrower address logic (non-critical)

Impact

Collateral claiming access control requirements differ for lender and borrower in removeCollateral, which increases the difficulty of usage of the system and makes it little more error prone.

Proof of Concept

lender is required to be to:

require(to == loan.lender, "NFTPair: not the lender");

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

While borrower has to be msg.sender:

require(msg.sender == loan.borrower, "NFTPair: not the borrower");

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

This way the logic isn't unified, while collateral retrieval action is the same.

Recommended Mitigation Steps

Consider unifying the approach, for example require both lender and borrower to be msg.sender, providing any to address.

4. Cook function doesn't check argument arrays length before accessing (non-critical)

Impact

On calling with arrays of different lengths various malfunctions are possible as the arrays are used as given. System then will fail with low level array access message.

Proof of Concept

cook operate the arrays without validation:

    function cook(
        uint8[] calldata actions,
        uint256[] calldata values,
        bytes[] calldata datas
    ) external payable returns (uint256 value1, uint256 value2) {
        for (uint256 i = 0; i < actions.length; i++) {

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

Recommended Mitigation Steps

Require that lengths of actions, values and datas arrays match.

5. User facing functions miss emergency lever (non-critical)

Impact

If there be any emergency with system contracts or outside infrastructure, there is no way to temporary stop the operations.

Proof of Concept

Most core user-facing contracts do not have pausing functionality for new operation initiation.

For example, NFTPair and NFTPairWithOracle user-facing requestLoan, lend and cook functions cannot be temporary paused:

requestLoan:

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

lend:

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

cook:

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

Recommended Mitigation Steps

Consider making all new actions linked user facing functions pausable, first of all ones without access controls.

For example, by using OpenZeppelin's approach:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol