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:
Borrower creates the loan with the knowledge that originationFeeRate is 1%
Owner of nfTLoanFacilitator contract increases originationFeeRate to 5% through updateOriginationFeeRate function (or performs frontrun with this transaction)
Lender lends money to the borrower through lend function and 5% of the transferred amount is charged as origination fee
Borrower got charged 5% instead of 1% he agreed to
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.
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.
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.
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 maximum5%. 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 with1%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 to5%and borrower is charged the fee he did not agree to.Attack scenario:
originationFeeRateis1%nfTLoanFacilitatorcontract increasesoriginationFeeRateto5%throughupdateOriginationFeeRatefunction (or performs frontrun with this transaction)lendfunction and5%of the transferred amount is charged as origination fee5%instead of1%he agreed toProof of Concept
Tools Used
Manual Review / VSCode
Recommended Mitigation Steps
It is recommended to add
originationFeeRatetoloancreated 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.solcontract 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
collateralContractAddressaddress - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L70loanAssetContractAddressaddress - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L73mintBorrowTicketToaddress - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L75sendCollateralToaddress - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L116sendLendTicketToaddress - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L134sendCollateralToaddress - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L253_contractaddress - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L279_contractaddress - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L289toaddress - https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L296Tools 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.solandPaladinRewardReserver.solcontracts 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
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.solcontract into proper/functional state requires multiple transactions:There is time frame where the value of lendTicketContract or borrowTicketContract is set to
0which might expose contract to some attacks and undefined behavior.Proof of Concept
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.