Usage of `transferFrom` leads to irreversible loss of user funds.

[code423n4]

Lines of code

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L213

Vulnerability details

[H] : Usage of transferFrom leads to irreversible loss of user ERC721 funds.

Assessed risk: 8/10

Urgency: 10/10

Codebase frequency: 1

[H - Impact]:

The validReceive modifier that checks whether the transaction of ERC721 or ERC1155 tokens has been executed according to the rules(msg.sender == underlying and operator == address(this)) is used on functions that would handle the callback on safeTransferFrom but it can be bypassed by a user that transfers on their own, via transferFrom .

The onERC721Received is only triggered via its subsequent safeTransferFrom function. Should a user transfer their tokens via transferFrom or any other transfer function facilitated by usual ERC721 contracts, the ERC721 token that they’ve just transferred would be lost, since the onERC721Received would not trigger to deny the improper(according to the implemented modifier) ERC721 transfer. The user would basically “forcibly” transfer their token, unknowingly that this is not the correct procedure of how the protocol should work.

[H - Mitigation]:

Although there’s not much that can be done to refuse the transferFrom transaction, I would suggest considering the following mitigation, for the sake of the users:

• implementing a “sweep”-like function; this must ensure that the token that the owner of the contract is about to send is not the actual underlying(so no shady business allowed). Moreover, it would transfer the _token _id that the user has previously wrongfully transferred. This way, the admins can verify on etherscan that the transaction happened wrongfully beforehand, confirm with the poor user, and send their token back! :). The mitigation could look something like this

function retrieveLostERC721(address _token, address _sender, uint256 _id) external onlyOwner {
    // making sure that the _token is not the underlying, so that no owner could
    // withdraw at will
    require(_token != underlying, "cannot withdraw underlying");

    ERC721(_token).safeTransferFrom(address(this), _sender, _id);
}

• I would also suggest adding warnings for the user, such that they do not transfer any type of tokens on their own, and use the actual protocol to do that.

[bunkerfinance-dev]

This isn't a priority for us because we can add a sweep function to the contract in the future.

[gzeoneth]

Downgrading to Low / QA.

[gzeoneth]

Treating as warden's QA report.