code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L271-L315 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L505-L543 https://github.com/code-423n4/2022-04-abranft/blob/main/test/NFTPair.test.ts#L296-L304

Vulnerability details

Impact

In the tests you can find a comment describing the fee structure of the contract:

      // The lender:
      // - Lends out the total
      // - Receives the open fee
      // - Pays the protocol fee (part of the open fee)
      // The borrower
      // - Receives the total
      // - Pays the open fee
      // The contract
      // - Keeps the protocol fee

barrylyndon from the Abracadabra team confirmed that the comment is still valid in a DM I sent him.

But, looking at the code you can see that the fees are actually collected and distributed a little differently:

The lender pays and receives the correct amount. But, the borrower doesn't only pay the open fee but also pays another fee to the protocol derived from the interest they pay. The protocol receives fees twice from both the lender and the borrower. The borrower isn't supposed to pay any tho. They lose part of their funds so I rate this issue as high.

Proof of Concept

I will show it for NFTPair but the same thing is valid for NFTPairWithOracle since it's pretty much the same contract.

In L290-L293 the total amount of shares and fees are determined:

        uint256 totalShare = bentoBox.toShare(asset, params.valuation, false);
        // No overflow: at most 128 + 16 bits (fits in BentoBox)
        uint256 openFeeShare = (totalShare * OPEN_FEE_BPS) / BPS;
        uint256 protocolFeeShare = (openFeeShare * PROTOCOL_FEE_BPS) / BPS;

In L295-L302 we can see that the contract receives the correct amount:

        if (skim) {
            require(
                bentoBox.balanceOf(asset, address(this)) >= (totalShare - openFeeShare + protocolFeeShare + feesEarnedShare),
                "NFTPair: skim too much"
            );
        } else {
            bentoBox.transfer(asset, lender, address(this), totalShare - openFeeShare + protocolFeeShare);
        }

The lender is supposed to receive the openFeeShare so you subtract it from the total amount of shares you take from them. The protocolFeeShare is paid by the borrower.

The totalShare - openFeeShare is then sent to the borrower in L304-L305:

uint256 borrowerShare = totalShare - openFeeShare;
bentoBox.transfer(asset, address(this), loan.borrower, borrowerShare);

When the borrower repays the loan, they pay the principal, interest, and a protocol fee that is derived from the interest as seen in L515-L524:

        uint128 principal = loanParams.valuation;

        // 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(principal, uint64(block.timestamp - loan.startTime), loanParams.annualInterestBPS).to128();
        uint256 fee = (interest * PROTOCOL_FEE_BPS) / BPS;
        amount = principal + interest;

        uint256 totalShare = bentoBox.toShare(asset, amount, false);
        uint256 feeShare = bentoBox.toShare(asset, fee, false);

The contract keeps the feeShare for itself and transfers the rest back to the lender, L526-L539:

        address from;
        if (skim) {
            require(bentoBox.balanceOf(asset, address(this)) >= (totalShare + feesEarnedShare), "NFTPair: skim too much");
            from = address(this);
            // No overflow: result fits in BentoBox
        } else {
            bentoBox.transfer(asset, msg.sender, address(this), feeShare);
            from = msg.sender;
        }
        // No underflow: PROTOCOL_FEE_BPS < BPS by construction.
        feesEarnedShare += feeShare;
        delete tokenLoan[tokenId];

        bentoBox.transfer(asset, from, loan.lender, totalShare - feeShare);

Thus, the protocol received fees twice. Once from the lender when _lend() is called and once from the borrower when repay() is called.

Tools Used

none

Recommended Mitigation Steps

When repaying, the contract shouldn't take any fees as described in the test's comment.

cryptolyndon commented 2 years ago

Not a bug, this is exactly how the contract is meant to work.

Borrowers pay an immediate fee when taking out a loan, and they pay interest when paying off the loan. The protocol takes a cut of both.

The comment is in a section of code that only calculates what the initial distribution of funds should look like, at the moment the loan is established.

0xean commented 2 years ago

downgrading to QA and duplicating with wardens QA report of #97

This is a documentation / comments issue

code-423n4 / 2022-04-abranft-findings