code-423n4 / 2022-03-joyn-findings

4 stars 1 forks source link

`mintToken` will fail on non-standard token #85

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L162

Vulnerability details

Proof of Concept

A simple POC: https://gist.github.com/wuwe1/9eb5bf9e4b3f31c8db52f4fa7fac5b13

Same reason as transferFrom in withdraw, but this time no fund lock. When dealing with non-standard token like USDT (no return value), this call will always revert.

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L162

    function mintToken(
        address to,
        bool isClaim,
        uint256 claimableAmount,
        uint256 amount,
        bytes32[] calldata merkleProof
    ) external onlyInitialized {

        -- snip --

        if (isClaim) {

        -- snip --

        } else {
            require(isForSale, "CoreCollection: Not for sale");
            if (mintFee > 0) {
                _handlePayment(mintFee * amount); // revert on non standard token
            }
        }

        batchMint(to, amount, isClaim);
    }

Recommended Mitigation Steps

Use safeERC20

sofianeOuafir commented 2 years ago

duplicate of #52

deluca-mike commented 2 years ago

Upon rereading this, this is invalid because mintToken will not revert as described. In fact, the return value of a noncompliant ERC20 transfer is ignored by the code, so it will never revert for the reasons described.