search

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

1 stars 1 forks source link

Burning `collateralContractAddress` by mistake in `closeLoan` #115

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

closeLoan; L116-216

Vulnerability details

Impact

ERC721 used as collateral could possibly never return to borrower.

Proof of Concept

No zero address check for sendCollateralTo might lead to sending ERC721 used as collateral to inexistent address. Use of transferFrom instead of safeTransferFrom doesn't check if ERC721 has been properly received.

Allowing users to send their tokens to oblivion (even if by mistake) can cause major reputation damage, given such tokens might be extremely valuable.

Tools Used

Recommended Mitigation Steps

Implement zero address check and/or use safeTransferFrom instead of transferFrom.

wilsoncusack commented 2 years ago

ERC721 standard states that transferFrom Throws if_tois the zero address, so 0 address is checked.

This has been brought up in several tickets and I mentioned that we feel users passing custom params here should know what they are doing and are not going to have the gas overhead of safeTransferFrom. Mentioned elsewhere the even uniswap uses _mint rather than _safeMint https://github.com/Uniswap/v3-periphery/blob/main/contracts/NonfungiblePositionManager.sol#L156

gzeoneth commented 2 years ago

Duplicate of #83