Staked service will be irrecoverable by owner if not an ERC721 receiver

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingBase.sol#L864

Vulnerability details

The StakingBase contract allows users to stake services represented by ERC721 tokens. These services are freely transferable, meaning they could be staked by contracts that do not implement the ERC721TokenReceiver interface.

When a user calls unstake() to withdraw their staked service, the contract attempts to transfer the ERC721 token back to the msg.sender (which must match the original depositor) using safeTransferFrom():

File: StakingBase.sol
862:         // Transfer the service back to the owner
863:         // Note that the reentrancy is not possible due to the ServiceInfo struct being deleted
864:         IService(serviceRegistry).safeTransferFrom(address(this), msg.sender, serviceId);

However, if the owner is a contract that does not support receiving ERC721 tokens, this transfer will fail. As a result, the service associated with serviceId will be permanently locked in the StakingBase contract, and the original owner will be unable to recover it.

Impact

Staked services will become permanently irrecoverable if the owner cannot receive ERC721 tokens.

Proof of Concept

1. User stakes a service by calling stake(serviceId) on the StakingBase contract from a smart contract that is not an ERC721TokenReceiver, and cannot be upgraded to support this (e.g. a Safe with restricted privileges). The service is transferred from the user to the contract.
2. When the user later tries to unstake and recover their service by calling unstake(serviceId), the safeTransferFrom() call in the function reverts.
3. The service remains locked in the StakingBase contract indefinitely, and the user is unable to recover it.

Tools Used

Manual review

Recommended Mitigation Steps

Consider using transferFrom() instead of safeTransferFrom() when transferring the service back to the owner in unstake(). This will allow the transfer to succeed even if the recipient is not an ERC721 receiver.

Assessed type

ERC721

[kupermind]

msg.sender is the owner that stakes the service, and it's exactly the same owner that is able to unstake it. What is the scenario that msg.sender was able to be the owner of the service before staking it, but is no longer able to receive ERC721 token when unstaking?

Our protocol assumes a valid ERC721 standard used in all the possible service registry contracts. If someone decides to use a custom broken ERC721 contract that is able to mint tokens to the contract, but cannot receive one by the transfer, this is out of scope of our protocol.

[c4-sponsor]

kupermind (sponsor) disputed

[kupermind]

After several communication rounds we can accept the issue, however the declared severity completely does not match the issue. There is zero risk factor for the protocol, only the user stupidity that have not checked the staking contract requirements.

None of well-known contract wallets reject the contract ERC721 support. This must be an artificially created scenario when the user deliberately uses the contract without such a support. If one is talking about outdated Safe, for example, there is a way to upgrade the Safe version, and then provide the correct fallbackHandler contract address to deal with ERC721 contracts.

[c4-judge]

0xA5DF changed the severity to 2 (Med Risk)

[c4-judge]

0xA5DF marked the issue as satisfactory

[c4-judge]

0xA5DF marked the issue as selected for report

[0xA5DF]

I agree that this is less likely to happen and somewhat on the user to ensure that their contract can receive ERC721 However, I think that the likelihood is sufficient to consider this as med, given the high impact.

[thebrittfactor]

For transparency, per discord discussion with the Olas sponsor (kupermind) the labeling has been updated to sponsor acknowledged.

[vsharma4394]

Fixed