Backed Protocol contest - reassor's results

1. Floating Lend Origination Fee

Risk

Low

Impact

Protocol takes origination fee rate on every lend that starts with 1% and can be updated at any time to maximum 5%. The fee is taken from the money sent by the lender to the borrower. The issue with this design is the floating origination fee. Borrower creates a loan and agrees with 1% lend origination fee, but that can be changed by the owner at any time, so at the time of receiving the loan, origination fee might be updated to 5% and borrower is charged the fee he did not agree to.

Attack scenario:

1. Borrower creates the loan with the knowledge that originationFeeRate is 1%
2. Owner of nfTLoanFacilitator contract increases originationFeeRate to 5% through updateOriginationFeeRate function (or performs frontrun with this transaction)
3. Lender lends money to the borrower through lend function and 5% of the transferred amount is charged as origination fee
4. Borrower got charged 5% instead of 1% he agreed to

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add originationFeeRate to loan created by borrower, so the user is charged exactly the origination fee he agreed to.

2. Missing zero address checks

Risk

Low

Impact

Multiple functions NFTLoanFacilitator.sol contract does not check for zero addresses which might lead to lost funds, failed transactions and can break the protocol functionality.

Proof of Concept

NFTLoanFacilitator.sol

• Setter missing check for collateralContractAddress address - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L70
• Setter missing check for loanAssetContractAddress address - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L73
• Setter missing check for mintBorrowTicketTo address - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L75
• Lost funds, missing check for sendCollateralTo address - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L116
• Lost funds, missing check for sendLendTicketTo address - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L134
• Lost funds, missing check for sendCollateralTo address - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L253
• Missing check for _contract address - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L279
• Missing check for _contract address - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L289
• Lost funds, missing check for to address - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L296

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended at the minimum to add zero address checks for all listed setter functions. In addition adding checks for functions that are using user's supplied addresses could prevent some unexpected behavior of the contracts.

3. Owner - critical address change

Risk

Low

Impact

Changing critical addresses such as ownership of HolyPaladinTokens.sol and PaladinRewardReserver.sol contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to implement two-step process for passing ownership for NFTLoanFacilitator.sol.

4. Transaction order dependence

Risk

Non-Critical

Impact

Deploying and setting NFTLoanFacilitator.sol contract into proper/functional state requires multiple transactions:

• deploy the contract
• set the lend ticket contract address
• set the borrow ticket contract address

There is time frame where the value of lendTicketContract or borrowTicketContract is set to 0 which might expose contract to some attacks and undefined behavior.

Proof of Concept

• https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L279-L283
• https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L289-L293

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to consider deploying contracts as a single transaction, for example by using factory contract.