search

code-423n4 / 2022-04-backed-findings

1 stars 1 forks source link

QA Report #52

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

The overall Backed codebase quality is good. Functionalities are very well documented; explanatory comments are well placed. State changing functions follow the Checks-Effects-Interactions pattern and properly validate inputs. Tests are comprehensive.

The permissionless nature of the system allows anyone to use malicious NFTs/collaterals in the system to trick users or cause irregular states. Consider applying a guarded launch approach by having a whitelist—or alternatively—a blacklist for certain NFTs/collaterals. It can be deactivated after the protocol has been sufficiently battle-tested in production.

Low Risk Vulnerabilities

1. Missing sanity check on minDurationSeconds

Mistakenly inputting a very low value of minDurationSeconds when creating a loan could lead to the NFT being lent and seized before the borrower is able to react.

Mitigation

Consider adding a reasonable minimum duration check when creating a loan.

2. Missing safeTransferFrom in closeLoan

Unlike seizeCollateral which implements safeTransferFrom, closeLoan only uses safeTransfer which could lead to irregular state for contract recipient.

Mitigation

Recommend using safeTransferFrom to transfer out the NFT when closing loan.

3. Unclear updateRequiredImprovementRate example

Comments on updateRequiredImprovementRate stated that E.g. setting this value to 10 would set improvement rate to 10%, while in reality inputting 10 would set the rate to 1%.

Mitigation

Consider updating the comment example to include the scale calculation.

4. Missing support for legacy NFTs

Popular legacy NFT like CryptoPunks (which is used as the mock NFT when running tests) doesn't conform to ERC721 standard, which will exclude them from being able to be loaned.

Mitigation

Consider adding a transfer module or utility function that could handle transferring of non-standard ERC721 NFTs.

Non Critical Vulnerabilities

1. Redundant variable naming

Redundant variable names collateralContractAddress and loanAssetContractAddress make the code less readable. Consider removing the ContractAddress as the type and name already imply it is a contract address.

wilsoncusack commented 2 years ago
  1. Won't do
  2. Likely will switch all of these to transferFrom
  3. Good catch!
  4. won't do
  5. ack