nikhil840096 - MlumStaking:safeTransferFrom will not work as expected hence failing in transfer of TokenId.

[sherlock-admin4]

nikhil840096

Medium

MlumStaking:safeTransferFrom will not work as expected hence failing in transfer of TokenId.

Summary

MlumStaking:safeTransferFrom is made to transfer the Position's tokenId .

Vulnerability Detail

MlumStaking:safeTransferFrom overrides ERC721Upgradeable and IERC721, also MlumStaking:transferFrom overrides ERC721Upgradeable, IERC721 Now let's go with the execution. Aslo note that MlumStaking:safeTransferFrom and MlumStaking:transferFrom has nonReentrant modifier.
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L336-L342
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L325-L331

user => MlumStaking:safeTransferFrom => ERC721Upgradeable.safeTransferFrom => transferFrom(MlumStaking:transferFrom)// Here Execution halts due to non reentrant modifier both the function using

ERC721Upgradeable.safeTransferFrom calls ERC721Upgradeable.transferFrom this function is overriden by the MlumStaking:transferFrom so the execution will get here and will fail due to non-reentrant modifier. As if user tries to burn using safeTransferFrom , sending it to address(0), this functionality will not work, due to above explained reasons.

Impact

• Users will not be able to use the this functionality as expected , breaking the functionality of the contract.
• Functionality is not working as expected by the protocol.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L336-L342
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L325-L331

Tool used

Manual Review

Recommendation

[0xSmartContract]

If a function that uses the reentrancy modifier is called twice during the call, the transaction will revert, but I haven't seen a call sequence where this is possible, I'd like to see a working POC on the subject

I also think the probability of this happening is very low and there is no loss of funds

[0xSmartContract]

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L751-L765

As of the current codebase, transferring of the NFT itself is already forbidden. So invalid

(2024-06-magicsea - Readme)

Q: Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in
future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on
properties/invariants that should hold?
No