By frontrun arbitrary user can DOS and manipulate the token information

Severity

MED

Vulnerability Detail

In the MCV2 token contract, the createMultiToken() and createToken() functions use the _clone() function to create new MCV2 tokens. The _clone() function uses the create2() function to create a new token contract, and the salt value for the create2() function is the tp.symbol parameter.

An attacker could frontrun a createMultiToken() or createToken() call by observing the call and then sending their own transaction with the same tp.symbol parameter, but with an incorrect tp.uri or tp.name parameter. This would cause the original transaction to revert, and the token with the specified symbol would have the incorrect name and URI.

Impact

If an attacker is able to frontrun a transaction, all the creation tx can be DOSed and the user will not be able to create the token with the desired name and URI. This could be a major setback for users who are trying to create a new token project.

The vulnerability is also more likely to be exploited on chains with low transaction fees, such as BSC. This is because it is easier for attackers to frontrun transactions on chains with low transaction fees.

Links to affected code

mint.club-v2-contract/contracts/MCV2_Bond.sol

Line 235 in 25ed197

address token = _clone(TOKEN_IMPLEMENTATION, tp.symbol);

mint.club-v2-contract/contracts/MCV2_Bond.sol

Line 262 in 25ed197

address token = _clone(MULTI_TOKEN_IMPLEMENTATION, tp.symbol);

Recommendation

Instead of using only tp.symbol as salt use below code.

bytes32 salt = keccak256(abi.encodePacked(msg.sender, tp.symbol));

address token = _clone(TOKEN_IMPLEMENTATION, salt);
address token = _clone(MULTI_TOKEN_IMPLEMENTATION, salt);

Valid point!

We used deterministic address generation because we wanted to enforce the uniqueness of the token symbol on-chain, so we can format the token's URL like this: mint.club/base/BTC

Even though there might be no financial benefit for a DoS attack, someone could just want to disrupt our service by doing so.

I guess another solution for this problem is to introduce a small creation fee (updateable), so we can adjust those fees if such attacks happen.

Thank you for providing context of service, and creation fee would be a better solution in that context.

Done! What do you think about that fix?

I believe protocolBeneficiary should be payable:

    function _collectCreationFee(uint256 amount) internal {
        if (amount != creationFee) revert MCV2_Royalty__InvalidCreationFee();
        if (creationFee == 0) return;
-        (bool success, ) = protocolBeneficiary.call{value: amount}("");
+        (bool success, ) = payable(protocolBeneficiary).call{value: amount}("");
        if (!success) revert MCV2_Royalty__CreationFeeTransactionFailed();
    }

@0x3agle Oh, true. We will just use a wallet address for protocolBeneficiary for now, but if we want to set it as a contract address later, we need the payable() keyword, right?

Yup

@sydneyitguy LGTM
@0x3agle Thank you for your good comments