code423n4 commented 2 years ago

Handle

defsec

Vulnerability details

Impact

During the code review, It has been seen that merged tokenIds burned but position is not deleted from array mapping.

Proof of Concept

Navigate to the following function.

    function merge(uint256[] memory tokenIds_, address destination_) external returns (uint256 tokenId_) {
        uint256 count = tokenIds_.length;
        require(count > uint256(1), "MIN_2_TO_MERGE");

        uint256 points;

        // For each NFT, check that it belongs to the caller, burn it, and accumulate the points.
        for (uint256 i; i < count; ++i) {
            uint256 tokenId = tokenIds_[i];
            require(ownerOf(tokenId) == msg.sender, "NOT_OWNER");
            require(positionOf[tokenId].expiry == uint32(0), "POSITION_NOT_UNLOCKED");

            _burn(tokenId);

            points += _getPointsFromTokenId(tokenId);
        }

        // Mine a new NFT to the destinations, based on the accumulated points.
        _safeMint(destination_, tokenId_ = _generateNewTokenId(points));
    }

However, the position has not been deleted.

        delete positionOf[tokenId_];

Tools Used

None

Recommended Mitigation Steps

Consider to review merge function behaviour and consider to re-generate position for the new token.

deluca-mike commented 2 years ago

Positions are deleted upon unlocking. Only NFTs that have no position (i.e. positionOf[tokenId].expiry == uint32(0)) can be merged.

Ivshti commented 2 years ago

agreed, not a bug

code-423n4 / 2022-01-xdefi-findings