Loss of users collateral/token directly sent to treasury address

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

Description: Description\ If the user directly sent the collateral or tokens to treasury address bypassing the Hub contract then it would result in loss of users collateral as only Hub contract can mint the group circles to users after receiving the collateral. As per documentation:

if one sends collateral directly to the Standard Treasury without data or with incorrectly structured data, the treasury will reject the transfer to avoid that funds get lost as only the hub controls minting of group tokens

if one sends tokens to the treasury with the correct data structure, bypassing the hub contract, the collateral will be locked in the treasury/vault, as the hub will not mint your equivalent group Circles

_groupMint() used in groupMint() function is implemented as:

    function _groupMint(
        address _sender,
        address _receiver,
        address _group,
        uint256[] memory _collateral,
        uint256[] memory _amounts,
        bytes memory _data,
        bool _explicitCall
    ) internal {

      . . . some code . . . 

        // note: treasury.on1155Received must implement and unpack the GroupMintMetadata to know the group
        safeBatchTransferFrom(_sender, treasuries[_group], _collateral, _amounts, dataWithGroup);

        // mint group Circles to the receiver and send the original _data onwards
        _mintAndUpdateTotalSupply(_receiver, toTokenId(_group), sumAmounts, _data);
    }

This function first transfers the colletral from user to treasury vault contract and after that mint the group circle to user's receiver address.

    function _mintAndUpdateTotalSupply(address _account, uint256 _id, uint256 _value, bytes memory _data) internal {
@>>>        _mint(_account, _id, _value, _data);

        uint64 today = day(block.timestamp);
        DiscountedBalance memory totalSupplyBalance = discountedTotalSupplies[_id];
        uint256 newTotalSupply =
            _calculateDiscountedBalance(totalSupplyBalance.balance, today - totalSupplyBalance.lastUpdatedDay) + _value;
        if (newTotalSupply > MAX_VALUE) {
            // DiscountedBalances: balance exceeds maximum value
            revert CirclesDemurrageAmountExceedsMaxUint190(_account, _id, newTotalSupply, 2);
        }
        totalSupplyBalance.balance = uint192(newTotalSupply);
        totalSupplyBalance.lastUpdatedDay = today;
        discountedTotalSupplies[_id] = totalSupplyBalance;
    }

Impact\ Due to non-restriction on use of safeBatchTransferFrom() in Hub.sol, if the user directly use it then his collateral would be lost. This is also mentioned in documenatation.

The impact is loss of users collateral or stucked of tokens, therefore issue is high severity.

Recommendation to fix Restrict the access of hub.safeBatchTransferFrom() function to treasury of vault address i.e it should only be called by StandardTreasury.sol contract to prevent this issue.

For example, burn() in Hub.sol can only be called by treasury of vault address i.e StandardTreasury.sol which can be seen as below:

    function burn(uint256 _id, uint256 _amount, bytes calldata _data) external {
        . . . some code . . . 

        IMintPolicy policy = IMintPolicy(mintPolicies[group]);
@>        if (address(policy) != address(0) && treasuries[group] != msg.sender) {

hats-finance / Circles-0x6ca9ca24d78af44582951825bef9eadcb210e5cf

Loss of users collateral/token directly sent to treasury address #55