Open code423n4 opened 2 years ago
The lender parameters are there as a guard against borrowers front running lend()
and changing the terms, not for the lender to update the terms. The expected situation is that the terms passed by the lender are an exact match. However, we felt that if the borrower did change the terms unbeknownst to the lender, but in a way that benefits the lender, then the lender deserves that benefit.
I was going to dismiss the first finding also, but your further justification changed my mind. Spending gas is unfair to callers of the original, hopefully non-botched, version IMO, but a comment is reasonable.
Attacker may call collateral/BentoBox before they are initialized
The
_call
function checks that the external call is not to BentoBox/collateral contracts, as then an attacker could call them and approve himself to spend NFTPair's funds:These address variables are being set in the
init
function. If an attacker callscall
before theinit
function has been called, he may execute the aforementioned malicious code. Now, I realize this will probably not happen with AbraNFT, as you are deploying the NFTPair using Bento'sdeploy
, which initializes the NFTPair atomically. So this can not happen. However, the risk is there in case somebody forks your project and changes the flow, as we've seen for example in the various Compund hacks and it's tech debts. So while you are basically not as risk, I suggest you mediate this vector for the potential good of humankind. You can do so by requiring thatcollateral != 0
etc' or by adding a comment notifying about the risk. May all human beings be happy.Lend function not updating loan parameters?
The lend function takes as an argument from the lender the loan params, and checking that they are not worse for the borrower. It then uses the parameters set by the borrower to initialite the loan, and not these parameters from the lender. I think it is a design choice as to which version to use, but just wanted to point this out in case this is by mistake.