0xeix - ERC721Pool.sol doesn't implement onERC721Received()

[sherlock-admin]

0xeix

high

ERC721Pool.sol doesn't implement onERC721Received()

Summary

ERC721Pool.sol that manages user's collateral doesn't have the function to actually receive tokenId

Vulnerability Detail

When we transfer tokenId to the contract, it should implement the function onERC721Received() to receive it as it doesn't use it by default (only EOA doesn't need it). And if there is no, any tokenIds sent will be stuck and lost.

Impact

High. Any tokenId that is sent to the pool will be lost.

Code Snippet

_transferFromSenderToPool() call in drawDebt():

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/ERC721Pool.sol#L198

_transferFromSenderToPool() call in addCollateral():

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/ERC721Pool.sol#L323

_transferNFT() function: https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/ERC721Pool.sol#L622

Tool used

Manual Review

Recommendation

Implement onERC721Received()

[grandizzy]

Any tokenId that is sent to the pool will be lost. - that's not valid as it implies the pool logic doesn't know how to handle NFTs

[0xffff11]

Seems invalid to me too. Because the pool clearly handles NFTs @grandizzy Might be low, the most accurate here, or not even?

[grandizzy]

Seems invalid to me too. Because the pool clearly handles NFTs @grandizzy Might be low, the most accurate here, or not even?

I suggest to close this one as the report is invalid, instead we should open https://github.com/sherlock-audit/2023-04-ajna-judging/issues/17 as documentation needs to be improved and could be an issue if for example one pulls collateral to a recipient that doesn't handle NFTs https://github.com/ajna-finance/ajna-core/blob/main/src/ERC721Pool.sol#L228

[0xffff11]

Agree with invalid