cocacola - safeTransferFrom function will always revert with ReentrancyGuardReentrantCall error

[sherlock-admin3]

cocacola

Medium

safeTransferFrom function will always revert with ReentrancyGuardReentrantCall error

Summary

The MlumStaking contract overrides both safeTransferFrom and transferFrom functions and adds nonReentrant them to prevent reentrancy attacks. However, internally, the OpenZepplin's ERC721 implementation calls transferFrom within the safeTransferFrom function. Thus, it is not possible to call safeTransferFrom function successfully, as it will revert every time due to overlapping nonReentrant modifiers.

Vulnerability Detail

The OpenZepplin's ERC721 implementation calls transferFrom within the safeTransferFrom function

    function transferFrom(address from, address to, uint256 tokenId) public virtual {
        if (to == address(0)) {
            revert ERC721InvalidReceiver(address(0));
        }
        address previousOwner = _update(to, tokenId, _msgSender());
        if (previousOwner != from) {
            revert ERC721IncorrectOwner(from, tokenId, previousOwner);
        }
    }

    function safeTransferFrom(address from, address to, uint256 tokenId) public {
        safeTransferFrom(from, to, tokenId, "");
    }

    function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual {
        transferFrom(from, to, tokenId);
        _checkOnERC721Received(from, to, tokenId, data);
    }

The safeTransferFrom and transferFrom functions and adds nonReentrant them to prevent reentrancy attacks.
Thus, it is not possible to call safeTransferFrom function successfully, as it will revert every time due to overlapping nonReentrant modifiers.

    /**
     * @dev Add nonReentrant to ERC721.transferFrom
     */
    function transferFrom(address from, address to, uint256 tokenId)
        public
        override(ERC721Upgradeable, IERC721)
        nonReentrant
    {
        ERC721Upgradeable.transferFrom(from, to, tokenId);
    }

    /**
     * @dev Add nonReentrant to ERC721.safeTransferFrom
     */
    function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory _data)
        public
        override(ERC721Upgradeable, IERC721)
        nonReentrant //@audit it will not work
    {
        ERC721Upgradeable.safeTransferFrom(from, to, tokenId, _data);
    }

The solution now prevents the transfer in general by overwriting _update, however, this particular implementation may affect future upgrades.

    function _update(address to, uint256 tokenId, address auth) 
        internal
        override(ERC721Upgradeable, ERC721EnumerableUpgradeable)
        returns (address)
    {
        address from = _ownerOf(tokenId);
        if (from != address(0) && to != address(0)) {
            revert("Forbidden: Transfer failed");
        }

        return super._update(to, tokenId, auth);
    }

Proof of Concept

    function testGT_safeTransferFrom_reverts() public {
        _rewardToken.mint(address(_pool), 100_000_000);
        _stakingToken.mint(ALICE, 2 ether);

        vm.startPrank(ALICE);
        _stakingToken.approve(address(_pool), 1 ether);
        _pool.createPosition(1 ether, 0);
        vm.stopPrank();

        assertEq(ERC721(address(_pool)).ownerOf(1), ALICE);

        vm.startPrank(ALICE);
        vm.expectRevert(ReentrancyGuard.ReentrancyGuardReentrantCall.selector);
        MlumStaking(address(_pool)).safeTransferFrom(ALICE, BOB, 1);
        vm.stopPrank();
    }

Impact

Users cannot benefit from safeTransferFrom function and callback check.

Code Snippet

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

Tool used

Manual Review

Recommendation

It is recommended to remove redundant nonReentrant modifier from the safeTransferFrom function.

Duplicate of #489

[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