AbraNFT contest - gzeon's results

Lines of code

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

Vulnerability details

Impact

In NFTPairWithOracle._lend, params.oracle is not checked. This allow a borrower to watch the mempool and front-run the lender's call and change oracle to avoid liquidation.

Proof of Concept

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

    function _lend(
        address lender,
        uint256 tokenId,
        TokenLoanParams memory accepted,
        bool skim
    ) internal {
        TokenLoan memory loan = tokenLoan[tokenId];
        require(loan.status == LOAN_REQUESTED, "NFTPair: not available");
        TokenLoanParams memory params = tokenLoanParams[tokenId];

        // Valuation has to be an exact match, everything else must be at least
        // as good for the lender as `accepted`.
        require(
            params.valuation == accepted.valuation &&
                params.duration <= accepted.duration &&
                params.annualInterestBPS >= accepted.annualInterestBPS &&
                params.ltvBPS >= accepted.ltvBPS,
            "NFTPair: bad params"
        );

Recommended Mitigation Steps

        require(
            params.valuation == accepted.valuation &&
                params.oracle == accepted.oracle &&
                params.duration <= accepted.duration &&
                params.annualInterestBPS >= accepted.annualInterestBPS &&
                params.ltvBPS <= accepted.ltvBPS,
            "NFTPair: bad params"
        );

Lines of code

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

Vulnerability details

Impact

In NFTPairWithOracle.updateLoanParams, a lender is allowed change the oracle. If the lender set it some oracle that return invalid price, he can call removeCollateral immediately to liquidate the borrower.

Proof of Concept

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

    function updateLoanParams(uint256 tokenId, TokenLoanParams memory params) public {
        TokenLoan memory loan = tokenLoan[tokenId];
        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/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L288-L289

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

Recommended Mitigation Steps

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

Lines of code

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

Vulnerability details

Impact

In NFTPairWithOracle.updateLoanParams, a lender is allowed to decrease ltvBPS. If the lender set it to 0, he can call removeCollateral immediately to liquidate the borrower.

Proof of Concept

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

    function updateLoanParams(uint256 tokenId, TokenLoanParams memory params) public {
        TokenLoan memory loan = tokenLoan[tokenId];
        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/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L288-L289

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

Recommended Mitigation Steps

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

Lines of code

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

Vulnerability details

Impact

In NFTPairWithOracle._lend, the loan ltvBPS can be higher than the lender's accepted ltvBPS. This allow a borrower to watch the mempool and front-run the lender's call and change ltvBPS to some very large value using updateLoanParams to avoid liquidation.

Proof of Concept

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

    function _lend(
        address lender,
        uint256 tokenId,
        TokenLoanParams memory accepted,
        bool skim
    ) internal {
        TokenLoan memory loan = tokenLoan[tokenId];
        require(loan.status == LOAN_REQUESTED, "NFTPair: not available");
        TokenLoanParams memory params = tokenLoanParams[tokenId];

        // Valuation has to be an exact match, everything else must be at least
        // as good for the lender as `accepted`.
        require(
            params.valuation == accepted.valuation &&
                params.duration <= accepted.duration &&
                params.annualInterestBPS >= accepted.annualInterestBPS &&
                params.ltvBPS >= accepted.ltvBPS,
            "NFTPair: bad params"
        );

Recommended Mitigation Steps

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

Low

init of the master contract can be called by anyone

While there seems to be no exploit, we can set collateral to a non-zero value in constructor to reduce risk. https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L175

Lack non-zero check of asset

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

    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");
    }

Collateral can be stuck if the to address not support ERC721

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

        collateral.transferFrom(address(this), to, tokenId);

Non-Critical

Upgrade Solidity Version

Consider to pin Solidity version to latest 0.8.12

Revert without reason

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

For loop optimization

Use ++i instead of i++ can save gas

contracts/NFTPairWithOracle.sol:527:        for (uint256 k = 2; k <= COMPOUND_INTEREST_TERMS; k++) {
contracts/NFTPairWithOracle.sol:674:        for (uint256 i = 0; i < actions.length; i++) {
contracts/NFTPair.sol:494:        for (uint256 k = 2; k <= COMPOUND_INTEREST_TERMS; k++) {
contracts/NFTPair.sol:641:        for (uint256 i = 0; i < actions.length; i++) {

Use different n depending on interest rate

For interest rate < 20%, N = 2 is enough https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L494-L495

        for (uint256 k = 2; k <= COMPOUND_INTEREST_TERMS; k++) {