User can reject the `withdrawalId` due to use of `_safeMint()` in `requestWithdrawal()` function

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x5529151e09b33810031083f111e067ffb0d1a1e92844738f22635f16497c35a2 Severity: medium

Description: Description\ Mintersol has requestWithdrawal() function where the user can request his base token withdrawal which is provided with redeem free management. Its implemented as:

    function requestWithdrawal(uint256 amount, address receiver) public nonReentrant {
        require(amount >= minWithdrawal, "LessThanMin");
        uint256 netAmount = previewWithdrawal(amount);
        stakingToken.safeTransferFrom(msg.sender, address(this), amount);

        uint256 withdrawalId = nextWithdrawalId++;
@>        _safeMint(receiver, withdrawalId);

        _withdrawalRequests[withdrawalId] = WithdrawalRequest({
            amount: netAmount,
            processed: false,
            claimed: false
        });

        totalPendingWithdrawals = totalPendingWithdrawals+netAmount;
        totalWithdrawalFees = totalWithdrawalFees+amount-netAmount;

        emit RequestWithdrawal(address(msg.sender), receiver, amount, withdrawalId);
    }

When the user requests for his withdrawal, they would first need to transfer the stakingTokens to contract. After that via safeMint(), a a NFT with incremented withdrawal id from previously minted NFT would be minted to user/receiver address.

_safeMint() is particularly used here to check the successful receival of NFT to contract address. However, requestWithdrawal() function can be exploited via _safeMint() function something similar to ShiUniverse incident.

Reference past attack with the use of openzeppelin's _safeMint() function- https://servrox-solutions.notion.site/Shi-Universe-Incident-03-07-2024-bc36aeb1124d4644b7e5342f299eca0c

The vulnerability can be arised with the use of _safeMint function, which is validates whether the recipient of an NFT is a smart contract. This function also checks if the smart contract has successfully received the NFT. This ensures that NFTs are not accidentally sent to non-existing or invalid contracts. However, There can be an exploit where the receiving smart contract could deliberately decline the reception of the NFT, causing the transaction to revert.

By knowing the NFT ID (withdrawalId) in advance, the receiving contract could continually reject undesired withdrawalId and only accept the NFT when the desired withdrawalId is received. This loophole allowed attackers to manipulate the minting process and selectively receive specific NFTs.

Recommendations\ Use _mint() instead of _safeMint()` to avoid above issue.

    function requestWithdrawal(uint256 amount, address receiver) public nonReentrant {
        require(amount >= minWithdrawal, "LessThanMin");
        uint256 netAmount = previewWithdrawal(amount);
        stakingToken.safeTransferFrom(msg.sender, address(this), amount);

        uint256 withdrawalId = nextWithdrawalId++;
-        _safeMint(receiver, withdrawalId);
+        _mint(receiver, withdrawalId);
        _withdrawalRequests[withdrawalId] = WithdrawalRequest({
            amount: netAmount,
            processed: false,
            claimed: false
        });

        totalPendingWithdrawals = totalPendingWithdrawals+netAmount;
        totalWithdrawalFees = totalWithdrawalFees+amount-netAmount;

        emit RequestWithdrawal(address(msg.sender), receiver, amount, withdrawalId);
    }

[aktech297]

@0xRizwan can you comment on the uniqueness of NFT here ? The reference made about the purposefully reverting in order to receive a specific NFT which is unique.

but here, there are no uniqueness, the NFT refers just an identity for depositing funds.

I am asking to know the extend of the NFT.

Also, note the economic cost of this revert is huge. as the attacker have to spend considerable amount of gas since the steps involved for the operation is more.

[ilzheev]

@0xRizwan https://github.com/AccumulatedFinance/contracts-v2/commit/066849ff477950d18e9dc1312fb4a82f80dc2f95