stake.link

Summary

Creating the lockId does not verify that the creator implements onERC721Received if it is a contract. Likewise, the transfer between chains of the ERC721 does not verify that the recipient is a contract.

Vulnerability Details

The creation of a lockId in the primary chain is with the help of the function SDLPoolPrimary::_storeNewLock, likewise in the secondary chain the function SDLPoolSecondary::_mintQueuedNewLocks is used. These functions assign the corresponding owner within the lockOwners[lockId]=owner. On the other hand, when sending an ERC721 from a primary chain to a secondary chain, a receiver can be specified and the receiver will get the token on the secondary chain with the help of the function SDLPoolSecondary::handleIncomingRESDL

The problem is that the functions don't check that if the owner is a contract, it should implement onERC721Received.

When transfering the lockId, if the receiver is a contract, the code does verify the implementation of onERC721Received with the help of SDLPool::_checkOnERC721Received, therefore when creating the lockId the implementation of onERC721Received should be verified if the new owner is a contract.

Impact

If the implementation of onERC721Received is not explicitly verified and the owner is a contract, it may lose the ERC721 token.

Tools used

Manual review

Recommendations

Add a validation in SDLPoolPrimary::_storeNewLock, SDLPoolSecondary: : _mintQueuedNewLocks and SDLPoolSecondary::handleIncomingRESDL to check the implementation of onERC721Received if the owner is a contract.