Logical flaw in isPermittedFlow function allows unintended flow permission

[hats-bug-reporter[bot]]

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

Description:

Description

The isPermittedFlow function executes multiple if statements, which can be broken down as follows:

        // Check if receiver trusts the Circles being sent
        if (uint256(trustMarkers[_to][_circlesAvatar].expiry) < block.timestamp) return false;

The first one simply returns false if the trust has already been expired

        // 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
        }

The second condition returns true if the advancedUsageFlags is set but the consented flow is not enabled.

        // 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)
        );

The third block first checks the expiry of the trust marker, followed by verifying the advancedUsageFlags for _to to ensure that consented flow is enabled. However, it neglects to check advancedUsageFlags[_from], incorrectly assuming that reaching this return statement means it is automatically set to true, while in reality, it could still be false.

This goes against the intended logic, as per the comments:

 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.

Specifically focusing on the point, "if the sender avatar has enabled consented flow," we can see that the sender's avatar may not have consented flow enabled and can still return true based on the third code block.

Since isPermittedFlow is used in critical functions like group minting and verifying the flow matrix, we deem this a high vulnerability.

Fix

return ( uint256(trustMarkers[_from][_to].expiry) >= block.timestamp
 && advancedUsageFlags[_to] & ADVANCED_FLAG_ENABLE_CONSENTEDFLOW != bytes32(0)
+ & advancedUsageFlags[_from]
);

Make sure that the sender has consented flow enabled

[benjaminbollen]

Thank you for your report on a potential logical flaw in the isPermittedFlow function. After careful review, we've determined that this is not a valid issue.

The function's logic is correct as implemented. If the code reaches the final return statement, it means that the previous condition was not met. Specifically, it indicates that the _from address has enabled the consented flow, which is the intended behavior.

We appreciate your thorough examination of our contract logic. While in this case the function is working as designed, your attention to detail in reviewing our code contributes to the overall security and robustness of our platform. Thank you for your efforts in helping ensure the integrity of our system.