Consented Flow Bypass In The `isPermittedFlow` Function

[hats-bug-reporter[bot]]

Github username: @minato7namikazi Twitter username: minato7namikazi Submission hash (on-chain): 0x105b60788a7965857696b866aee0f5b4ea8c47d61430952d3895110e3fae6fee Severity: high

Description: https://github.com/hats-finance/Circles-0x6ca9ca24d78af44582951825bef9eadcb210e5cf/blob/507e18587b8a0b64a4bb21db01ecf876dc607e47/src/hub/Hub.sol#L638

In The isPermittedFlow Function

 function isPermittedFlow(address _from, address _to, address _circlesAvatar) public view returns (bool) {
        // Check if receiver trusts the Circles being sent
        if (uint256(trustMarkers[_to][_circlesAvatar].expiry) < block.timestamp) return false;

        // Check if sender has enabled consented flow
        if (advancedUsageFlags[_from] & ADVANCED_FLAG_ENABLE_CONSENTEDFLOW == bytes32(0)) {
            return true; // If not enabled, standard trust is sufficient
        }
        // For consented flow, check sender trusts receiver,
        // and receiver has consented flow enabled too
        return (
            uint256(trustMarkers[_from][_to].expiry) >= block.timestamp
                && advancedUsageFlags[_to] & ADVANCED_FLAG_ENABLE_CONSENTEDFLOW != bytes32(0)
        );
    }

Description

The isPermittedFlow function contains a critical bug that allows unauthorized token transfers when the consented flow feature is enabled. The function fails to properly implement the additional trust checks required for consented flow, effectively negating the feature's purpose and potentially compromising user control over their tokens.

in the line:

if (uint256(trustMarkers[_to][_circlesAvatar].expiry) < block.timestamp) return false;

If the sender (_from) has enabled consented flow, we need to also check if the sender trusts the receiver and if the receiver has consented flow enabled.

Impact

This bug allows transfers to occur even when consented flow is enabled, without enforcing the additional trust checks. This undermines the purpose of the consented flow feature and breaks the core principle of the Circles system, which is built on trust relationships between users

Proof of Concept

1. Alice enables consented flow for her account.
2. Bob does not enable consented flow.
3. Charlie is a Circles avatar (token issuer).
4. Bob trusts Charlie's Circles.
5. Alice does not trust Bob.
6. Alice initiates a transfer of Charlie's Circles to Bob.
7. The isPermittedFlow(Alice, Bob, Charlie) function is called.
8. The function only checks if Bob trusts Charlie's Circles (which he does).
9. The function returns true, allowing the transfer to proceed.
10. The transfer succeeds, despite Alice not trusting Bob and having consented flow enabled.

This violates the intended behavior of the consented flow feature, which is supposed to provide additional control over token flows

Why This Could Be Triggered ?

This bug can be triggered in any situation where a user has enabled consented flow. The current implementation of isPermittedFlow only checks if the receiver trusts the Circles avatar, without considering the sender's consented flow status or trust relationship with the receiver.

Mitigation

To fix this issue, the isPermittedFlow function should be updated to include all necessary checks for consented flow :

function isPermittedFlow(address _from, address _to, address _circlesAvatar) public view returns (bool) {
    // Check if receiver trusts the Circles being sent
    if (uint256(trustMarkers[_to][_circlesAvatar].expiry) < block.timestamp) return false;

    // Check if sender has enabled consented flow
    if (advancedUsageFlags[_from] & ADVANCED_FLAG_ENABLE_CONSENTEDFLOW == bytes32(0)) {
        return true; // If not enabled, standard trust is sufficient
    }

    // For consented flow, check sender trusts receiver,
    // and receiver has consented flow enabled too
    return (
        uint256(trustMarkers[_from][_to].expiry) >= block.timestamp
        && advancedUsageFlags[_to] & ADVANCED_FLAG_ENABLE_CONSENTEDFLOW != bytes32(0)
    );
}

This suggestion :

1. Checks if the receiver trusts the Circles avatar (standard check).
2. Checks if the sender has enabled consented flow.
3. If consented flow is enabled, it additionally checks if: a. The sender trusts the receiver b. The receiver also has consented flow enabled

[minato7namikazi]

Sorry, an incorrect mitigation was sent by mistake

And I think I mixed up some information in the above report.

So, to be continued ...

[minato7namikazi]

According to the docs,

"If the sender avatar has enabled consented flow for the Circles balances they own, then the receiver must trust the Circles being sent, and the sender must trust the receiver, and to preserve the protection recursively the receiver themselves must have consented flow enabled."

The current function implementation

1. correctly checks if the receiver trusts the Circles being sent.
2. It correctly checks if the sender has enabled consented flow.
3. If consented flow is not enabled, it correctly returns true (standard trust is sufficient).
4. If consented flow is enabled, it correctly checks if the sender trusts the receiver and if the receiver has consented flow enabled.

But it's missing a check to ensure the sender trusts the Circles being sent when consented flow is enabled.

[minato7namikazi]

So because the concept of "consented flow" is an opt-in feature that provides additional control over how an avatar's tokens can be used in path-based transactions

And only when consented flow is enabled for a sender, the system should enforce stricter rules for token transfers.

Like the checks :

- The receiver trusts the Circles being sent
- The sender trusts the receiver (if consented flow is enabled)
- The receiver has consented flow enabled (if the sender has it enabled)

Missing whether the sender trusts the specific Circles tokens being sent

• is important because in the Circles ecosystem, trusting an avatar doesn't automatically mean trusting all Circles associated with that avatar. An avatar might trust another avatar but not necessarily all the different types of Circles that avatar might hold or try to transfer.

• By not checking if the sender trusts the specific Circles being sent, it could potentially allow transfers of Circles that the sender hasn't explicitly trusted, even when they've enabled the stricter consented flow feature.

• A user might trust another avatar but not necessarily all the different types of Circles that avatar might hold or try to transfer.

To fix this, an additional check should be added to verify that the sender trusts the specific _circlesAvatar (the avatar of the Circles being sent) when consented flow is enabled. This would ensure that users have granular control over which Circles can be used in transactions when they've opted for stricter controls.

[benjaminbollen]

this line checks "whether the receiver DOES NOT trust the tokens being sent, and if so, returns false early.

if (uint256(trustMarkers[_to][_circlesAvatar].expiry) < block.timestamp) return false;

so your PoC line 8 and 9, is just not evaluating this code

The function only checks if Bob trusts Charlie's Circles (which he does) The function returns true, allowing the transfer to proceed

[minato7namikazi]

Yup, the first report is mixed up

The only valid concerns are the last two comments

[benjaminbollen]

But it's missing a check to ensure the sender trusts the Circles being sent when consented flow is enabled.

right, I forgot to follow up. This is never a required check; if the sender has any Circles, they can be considered for a flow edge, with or without consented flow.