1. Unnecessary loan parameter comparison in _lend when used in through requestAndBorrow or takeCollateralAndLend
in internal function _lend, it takes an parameter called accepted and compare it with the state variable tokenLoanParams to make sure the borrower didn't front-run the tx. This is a good check, but in requestAndBorrow and takeCollateralAndLend, same parameters are passed in for _requestLoan and _lend, considering these are probably the most used function, it would be better to use a boolean call checkLoan to decide if we need this check or not. It will only be needed in lend function. (or just move the check into a different internal function and call it before lend)
QA Report: Recommandation (No bug found)
I highly recommend the team to remove the ability to do arbitrary function calls on behalf of the NFTPair contract. Use of arbitrary contract call is a very dangerous pattern, it should be avoided unless there's a very strong motive to support it.
Things that can go wrong with it:
user's wallet can be drained if they accidentally approve the NFTPair contract to use their asset. (it should have been approving BentoBox) This can be achieved by calling asset.transferFrom(user, attacker).
potential airdrops or any token will become public for anyone to withdraw.
it's possible that the collateral suffer the same vulnerability as the tUSD bug and could have lead to huge hack in Compound. The exact bug caused by having 2 entry points to a single contract is unlikely to happen with well-known ERC20s again, but there might be more potential issue similar to this one, and the process of adding collateral will need to be better reviewed, potentially the team should not accept any upgradable contracts.
Same thing happen to BentoBoxV1 contract, if the team decide to launch this code base on BentoBoxV1 it should be safe, but if the newer version is upgrdable then this mechanism cannot be trusted, because it's possible that user use different entry points to mess up BentoBox's accounting.
Solution:
If the team added this feature to support token farming or trapped tokens, it would make sense to add onlyOwner sweep function to do that; If it was mainly for interacting with other protocols, I suggest add a custom interface as a middle layer: So only allow the NFTPair contract to call ICallee(callee).ourSpecialCall(data), and for each cool feature you want to support you add a new callee contract which works as a security proxy.
IMO arbitrary calls lead to a huge space of potential vulnerabilities (some of them might still be unknown), so i would suggest to completely remove the possibility of volnerability by replacing it with one of the solution above.
cryptolyndon
commented
2 years ago
Gas optimizations:
1. Unnecessary loan parameter comparison in
_lendwhen used in throughrequestAndBorrowortakeCollateralAndLend_lend, it takes an parameter calledacceptedand compare it with the state variable tokenLoanParams to make sure the borrower didn't front-run the tx. This is a good check, but inrequestAndBorrowandtakeCollateralAndLend, same parameters are passed in for_requestLoanand_lend, considering these are probably the most used function, it would be better to use a boolean call checkLoan to decide if we need this check or not. It will only be needed inlendfunction. (or just move the check into a different internal function and call it beforelend)QA Report: Recommandation (No bug found)
I highly recommend the team to remove the ability to do arbitrary function calls on behalf of the NFTPair contract. Use of arbitrary contract call is a very dangerous pattern, it should be avoided unless there's a very strong motive to support it.
Things that can go wrong with it:
NFTPaircontract to use theirasset. (it should have been approving BentoBox) This can be achieved by calling asset.transferFrom(user, attacker).collateralsuffer the same vulnerability as the tUSD bug and could have lead to huge hack in Compound. The exact bug caused by having 2 entry points to a single contract is unlikely to happen with well-known ERC20s again, but there might be more potential issue similar to this one, and the process of adding collateral will need to be better reviewed, potentially the team should not accept any upgradable contracts.BentoBoxV1contract, if the team decide to launch this code base on BentoBoxV1 it should be safe, but if the newer version is upgrdable then this mechanism cannot be trusted, because it's possible that user use different entry points to mess up BentoBox's accounting.Solution:
If the team added this feature to support token farming or trapped tokens, it would make sense to add
onlyOwnersweep function to do that; If it was mainly for interacting with other protocols, I suggest add a custom interface as a middle layer: So only allow theNFTPaircontract to callICallee(callee).ourSpecialCall(data), and for each cool feature you want to support you add a new callee contract which works as a security proxy.IMO arbitrary calls lead to a huge space of potential vulnerabilities (some of them might still be unknown), so i would suggest to completely remove the possibility of volnerability by replacing it with one of the solution above.