AbraNFT contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L365-L386 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L405-L422

Vulnerability details

Impact

Right now this doesn't have critical impact, because sending zero address to _lend and _requestLoan will revert the transaction. but with bypassing signature check, if logics of those function changes, then this will cause a fund lose.

Proof of Concept

The code don't check that lender and borrower address is not zero. ecrecover() will return zero if the signature is not valid and attacker can send wrong signature with zero address for lender/browser and bypass signature check. Of course in the codes after this signature check, code will revert, but still the signature check logic is not complete.

Tools Used

VIM

Recommended Mitigation Steps

add this line to requestAndBorrow: require(address(lender) != address(0), "NFTPair: lender is zero");

and this line to takeCollateralAndLend: require(address(borrower) != address(0), "NFTPair: lender is zero");