search

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

0 stars 0 forks source link

No successful transfer check on _requestLoan #157

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Issue: No check for successful transfer of collateral. Nonstandard tokens which do not revert on transfer failure can lead to creating loan requests which have no backing.

Consequence: Loss of lender's funds if a bad loan is accepted.

Mitigations

Add require(collateral.ownerOf(tokenId) == address(this)); after the if/else statement.

cryptolyndon commented 2 years ago

Not an issue. Bad tokens that are nevertheless popular enough to warrant it can have their own contract; doing this would unduly punish everyone else.

0xean commented 2 years ago

@cryptolyndon - can you explain what you mean by punish everyone? Do you mean in terms of gas costs?

cryptolyndon commented 2 years ago

Yes

0xean commented 2 years ago

Per the 721 specs, I am going to side with the sponsor on this.

A failed transfer must throw for a NFT to comply with the spec

https://eips.ethereum.org/EIPS/eip-721

If the NFT doesn't throw on a failed transfer, its not compliant and therefore should not be used in a system expecting a compliant 721 NFT