code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L337 https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L344

Vulnerability details

Impact

During the code review, It has been observed the token does not have any maximum total supply definition on the code. With privileged user, the contract masterMinter can define any amount on the minting. The contract will have one masterMinter address. Any compromise to the masterMinter account(s) may allow the hacker to manipulate the project through these functions. (unlimited minting)

Proof of Concept

Navigate to the following contract function.

    function configureMinter(address minter, uint256 minterAllowedAmount)
        external
        whenNotPaused
        onlyMasterMinter
        returns (bool)
    {
        minters[minter] = true;
        minterAllowed[minter] = minterAllowedAmount;
        emit MinterConfigured(minter, minterAllowedAmount);
        return true;
    }

    function updateMasterMinter(address _newMasterMinter) external onlyOwner {
        require(
            _newMasterMinter != address(0),
            "FiatToken: new masterMinter is the zero address"
        );
        masterMinter = _newMasterMinter;
        emit MasterMinterChanged(masterMinter);
    }

Tools Used

Code Review

Recommended Mitigation Steps

We advise the client to carefully manage the onlyMasterMinter account private key to avoid any potential risks of being hacked. In general, we strongly recommend centralized privileges or roles in the protocol to be improved via a decentralized mechanism or smart-contract-based accounts with enhanced security practices, e.g., Multisignature wallets.

Define maximum total supply.
Indicatively, here is some feasible suggestions that would also mitigate the potential risk at the different level in term of short-term and long-term goal:
Time-lock with reasonable latency, e.g. 48 hours, for awareness on privileged operations; Assignment of privileged roles to multi-signature wallets to prevent a single point of failure due to the private key;
Introduction of a DAO/governance/voting module to increase transparency and user involvement.

retocrooman commented 2 years ago

There is no response this time because it is an operational problem. I don't think it's MedRisk because it's just a matter of proper operation. I think it's Low Risk.

jack-the-pug commented 2 years ago

Dup #42

CloudEllie commented 2 years ago

Grouping this with warden's QA report, #47

code-423n4 / 2022-02-jpyc-findings