code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L155

Vulnerability details

Some ERC20 tokens, such as Tether (USDT), allow for charging a fee any time transfer() or transferFrom() is called.

Impact

The code miscalculates whether the loan has been fully paid or bought out because it relies on the value of amounts passed in rather than the actual amount transferred in the call to safeTransferFrom()/safeTransfer(). The calculation should take into account the fees charged as a part of the transfer so that the protocol does not leak value.

Proof of Concept

The following calls will not result in the receiver getting the full amount if a fee-on-transfer token is used:

            ERC20(loanAssetContractAddress).safeTransferFrom(msg.sender, address(this), amount);

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L155

            ERC20(loanAssetContractAddress).safeTransfer(
                IERC721(borrowTicketContract).ownerOf(loanId),
                amount - facilitatorTake
            );

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L157-L160

                ERC20(loanAssetContractAddress).safeTransferFrom(
                    msg.sender,
                    address(this),
                    amount + accumulatedInterest
                );

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L200-L204

                ERC20(loanAssetContractAddress).safeTransfer(
                    currentLoanOwner,
                    accumulatedInterest + previousLoanAmount
                );

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L205-L208

                ERC20(loanAssetContractAddress).safeTransfer(
                    IERC721(borrowTicketContract).ownerOf(loanId),
                    amountIncrease - facilitatorTake
                );

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L210-L213

                ERC20(loan.loanAssetContractAddress).safeTransferFrom(
                    msg.sender,
                    currentLoanOwner,
                    accumulatedInterest + previousLoanAmount
                );

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L215-L219

        ERC20(loan.loanAssetContractAddress).safeTransferFrom(msg.sender, lender, interest + loan.loanAmount);

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L241

Tools Used

Code inspection

Recommended Mitigation Steps

Measure the balance before and after the calls to safeTransferFrom(), and use the difference between the two as the amount, rather than the amount stated

wilsoncusack commented 2 years ago

We plan to resolved #75 but are not concerned with this specifically. Since borrowers propose the token to use for loans, we believe it is on them to know the "gotchas" of the tokens they work with

gzeoneth commented 2 years ago

Duplicate of #75

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