code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/MINTR.sol#L33-L35 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/external/OlympusERC20.sol#L910-L912 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/external/OlympusERC20.sol#L779-L785

Vulnerability details

Permissioned actors can arbitrarily inflate OHM total supply to uint max in MINTR.sol.

Synopsis

A permissioned user can interact with the MINTR.sol function and call mintOhm() with the parameters 'to' being their address and 'amount' being uintMax - totalSupply.

Proof of concept.

User A (permissioned) calls MINTR.sol mintOhm() function, providing their own address as the 'to_' parameter and an amount of uint_max - ohm.totalSupply() (to inflate current supply to uintMax)

From the code:

    function mintOhm(address to_, uint256 amount_) public permissioned {
        ohm.mint(to_, amount_);
    }

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/MINTR.sol#L33-L35

The ohm.mint() call calls the ERC20Permit contract's mint function, which in turn calls the ERC20 inherited _mint() function.

    function _mint(address account, uint256 amount) internal virtual {
        require(account != address(0), "ERC20: mint to the zero address");
        _beforeTokenTransfer(address(0), account, amount);
        _totalSupply = _totalSupply.add(amount);
        _balances[account] = _balances[account].add(amount);
        emit Transfer(address(0), account, amount);
    }

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/external/OlympusERC20.sol#L910-L912

Within this function, beforeTokenTransfer() does no checking of the transaction for validation:

    function _beforeTokenTransfer(
        address from_,
        address to_,
        uint256 amount_
    ) internal virtual {}

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/external/OlympusERC20.sol#L779-L785

The total supply gets inflated and the balance is added to the user's account.

Resolutions and issues.

Even if this is a 'trusted' contract or address, there is no timelock on the action and therefore users' funds could be at serious risk should a permissioned actor turn nefarious. As such, I believe this could constitute a high risk vulnerability. A reasonable suggestion might be to incorporate a timelock, so users can escape prior to supply being diluted, however, the ensuing rapid decline of token value would be immense and potential investors could suffer significant losses.

Additionally, it does not seem a multi-sig wallet alone would be sufficient defence - if a sufficient number of members of the multi-sig signers wished to rug, it would be near instant and investors would already lose funds without warning.

Time lock, multi-sig and the inability to pass in an inflation to uint_max or near it might be useful here.

0xLienid commented 2 years ago

This is a known security assumption and is largely irrelevant as approved addresses will be policies with verifiable behavior around minting

0xean commented 2 years ago

Closing as invalid, this "vulnerability" is present in any token with a public mint function.

code-423n4 / 2022-08-olympus-findings