Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #104

Open
code423n4 opened this issue Apr 7, 2022 · 0 comments
Open

QA Report #104

code423n4 opened this issue Apr 7, 2022 · 0 comments
Labels
bug Something isn't working duplicate This issue or pull request already exists QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

QA report

Non-critical

Use safeTransferFrom for ERC721 consistency

In the contract NFTLoanFacilitator.sol there is a mix of using IERC721.safeTransferFrom() and IERC721.transferFrom(). The methods repayAndCloseLoan() and seizeCollateral() uses the safeTransferFrom(), where the methods createLoan() and closeLoan() uses the transferFrom().

For the method closeLoan() it should just be changed, since it transfers the collateral from the contract to another address just like the methods repayAndCloseLoan() and seizeCollateral(), which both uses the safeTransferFrom().

However, in order to change the method createLoan() to use the safeTransferFrom() it will require the contract to implement the IERC721Receive.onERC721Received() method.

Notice! Currently the method closeLoan() does not make use of the safeTransferFrom() and this means that if a user provided a contract address as the parameter sendCollateralTo and this contract does not implement proper handling of ERC721, then the token would be lost. But since I see this as very unlikely, I choose to consider this as a non-critical instead of a low.

@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Apr 7, 2022
code423n4 added a commit that referenced this issue Apr 7, 2022
@wilsoncusack wilsoncusack added duplicate This issue or pull request already exists sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working duplicate This issue or pull request already exists QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

2 participants