Two stX token minters cannot work simultaneously

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x21489a057fa0506a3a1faab5d479a4beea55f5f7f2a1c0f89530484024be14b5 Severity: high

Description: Description\

Minter.sol implements the logic for a MinterContract, which acts as an middleMan to mint staked tokens of a specific asset.

There is 1 base implementation and 2 implementations that can be used for all stX tokens that Accumulated Finance wants to support.

BaseMinter the abstract contract and:

• NativeMinter allowing depositing the NATIVE token of the chain and receiving stX (staked representation of the NATIVE token)
• ERC20Minter allowing to deposit baseToken and get back the staked version of the chain's NATIVE token

Both Minters will be different deployed contracts (eg - for ZETA NATIVE token - stZETAMinter.sol and stZETAMinterERC20.sol), both have a deposit() that will mint to the user stX (stZETA in this example), but since mint() inside stX tokens is onlyOwner, Minter must be the owner of the token. But a token can only have one owner at a time and therefore both ways of getting stX tokens are not possible, only one will work as the other Minter contract will not be the owner and stX.mint() will fail.

contract NativeMinter is BaseMinter {

    ...

    function deposit(address receiver) public payable virtual nonReentrant {
        require(msg.value > 0, "ZeroDeposit");
        uint256 mintAmount = previewDeposit(msg.value);
        require(mintAmount > 0, "ZeroMintAmount");
        stakingToken.mint(receiver, mintAmount); // AUDIT - will revert here
        emit Deposit(address(msg.sender), receiver, msg.value);
    }

    ...

}

// ERC20Minter contract accepts ERC20 token as a base token for liduid staking
contract ERC20Minter is BaseMinter {

    ...

    function deposit(uint256 amount, address receiver) public virtual nonReentrant {
        require(amount > 0, "ZeroDeposit");
        uint256 mintAmount = previewDeposit(amount);
        require(mintAmount > 0, "ZeroMintAmount");
        baseToken.safeTransferFrom(address(msg.sender), address(this), amount);
        stakingToken.mint(receiver, mintAmount); // AUDIT - will revert here
        emit Deposit(address(msg.sender), receiver, amount);
    }

    ...

}

contract stZETA is ERC20, ERC20Burnable, Pausable, Ownable {
    constructor(
        string memory name,
        string memory symbol,
        uint8 decimals
    ) ERC20(name, symbol, decimals) {}

    function pause() public onlyOwner {
        _pause();
    }

    function unpause() public onlyOwner {
        _unpause();
    }

    function mint(address to, uint256 amount) public onlyOwner {
        _mint(to, amount);
    }

    function _beforeTokenTransfer(address from, address to, uint256 amount)
        internal
        whenNotPaused
        override
    {
        super._beforeTokenTransfer(from, to, amount);
    }
}

Attack Scenario\ Describe how the vulnerability can be exploited.

Recommended Mitigation

This can be fixed by changing the withdraw() implementation of both minters to not be 1:1 as a signature and then have a contract that will inherit both minters and users can deposit there either base or NATIVE tokens, this way only this contract you need to be the owner of the stX token and you will mine it.

Another approach which is easier is to make mint() inside stX not onlyOwner but with a role check (MINTER_ROLE) and grant that role to both Minters.

[0xRizwan]

stZETAMinter.sol and stZETAMinterERC20.sol are OOS.

[Slavchew]

Hey @0xRizwan, thanks for the quick reply.

The problem above is in NativeMinter and ERC20Minter and that they cannot be used simultaneously, Zeta minters are just an example of a token, it doesn't matter which token. Also as you can see NativeMinter and ERC20Minter are the base ones and then every token implementation has 2 minters deriving from these contracts but they only change if the baseToken has something specific like in the ROSE case, all others are 1:1 with NativeMinter and ERC20Minter.

The vulnerable line is from NativeMinter and ERC20Minter, so that's why this issue is in scope and valid High.

[ilzheev]

@Slavchew Answered here – minters are not used simultaneously by design: https://github.com/hats-finance/OLD-Accumulated-finance-0x75278bcc0fa7c9e3af98654bce195eaf3bb6a784/issues/25#issuecomment-2325890860