Gas-griefing and re-entrancy on `treasury` and `mintPolicy` functions

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xefcb418ed363eeb90786d2a691e2549a39478f275e9ba99d122a8abed3508b62 Severity: medium

Description:

Impact

Gas-griefing: Loss of native token up to the block.gaslimit with each vulnerable transaction Potential loss of funds or value upon re-entrancy attacks

Note: as this vulnerability is by design of the current system I decided to submit it as a single medium severity combining both the gas-griefing and the re-entrancy issue into a single submission

Description

Groups are required to set up a mint policy that can be any address with registerGroup and also can set up an arbitrary treasury via registerCustomGroup. This flexibility gives way for a lot of attacks against users who attempt to interact with groups such as minting their circle.

Hub.sol - registerCustomGroup()

    function registerCustomGroup(
        address _mint,
        address _treasury,
        string calldata _name,
        string calldata _symbol,
        bytes32 _metadataDigest
    ) external {
        _registerGroup(msg.sender, _mint, _treasury, _name, _symbol);

        // for groups register possible custom name and symbol
        nameRegistry.registerCustomName(msg.sender, _name);
        nameRegistry.registerCustomSymbol(msg.sender, _symbol);

        // store the metadata digest for the group metadata
        nameRegistry.setMetadataDigest(msg.sender, _metadataDigest);

        emit RegisterGroup(msg.sender, _mint, _treasury, _name, _symbol);
    }

Vulnerable functions

The following functions called can spend up to the block.gaslimit funds of the sender and re-enter functions of circle-v2 contracts

• Hub.groupMint()
• treasury.onERC1155Received()
• policy.beforeMintPolicy()
• policy.beforeBurnPolicy()
• policy.beforeRedeemPolicy()

Proof of Concept

Here are two examples of these kind of attacks:

Group Mint re-entrancy Loss of Value

1. Hexagon group has a popular Circle token that has a trade-able market value of $10
2. User tries to mint large amount of 10000 hexagonCircle with valid collateral calling groupMint(): expects a $10 value of hexagonCircles to be minted aka total of $100k
3. When _groupMint() calls safeBatchTransferFrom() -> treasury.onERC1155Received() can immediately sell group owned hexagonCircles and collateral from the vault to dump the token price to $5
4. User now has 10000 hexagonCircles but with a value of only $50k instead of $100k

Gas-griefing burn and redeem policy

1. Hexagon group is incentivized to mint as many tokens for collateral tokens to their vault
2. They decided implement a MintPolicy that spends as much gas as possible on redeems and burns of the treasury to punish users who intend to redeem or burn their token
3. Any user who attempts to redeem and burn will pay gas costs up to the block.gaslimit because of arbitrary .beforeRedeemPolicy() and .beforeBurnPolicy() gas spends

Note they can also upgrade the MintPolicy if it's an upgradeable contract to suddenly gas-grief users

Recommendation

Consider to add reentrancy guards to all of the publicly available functions.

Consider to disallow arbitrary addresses as treasuries and mint policies as they can be very dangerous, instead try to design a more customize-able mint policy and treasury and deploy it during registerGroup(): e.g. only deployed policies and treasuries from the Hub would be enabled to used by the contracts. While this would reduce flexibility of the codebase, it would marginally improve safety.

[benjaminbollen]

Intended design to be an open ecosystem of custom smart contracts, so vetting of trustworthy contracts is a responsibility for wallets and community tools built.

On the second topic of re-entrancy, there is no harm as far as we can see without an explicit re-entrancy lock, nor does your issue highlight one.