Blacklisted user or proxy address can cause funds to be stuck in the contract forever

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L160

Vulnerability details

Impact

It has been stated by the sponsor that the protocol will accept any ERC20-compliant token that implements ERC20Votes. When a user wishes to delegate some of his tokens to increase a delegate voting power, he locks his tokens in a proxy contract. He can later, recover his tokens (or get reimbursed) from the proxy using the same delegateMulti function. When ERC20 token used for payment implements a blacklist like functionality, if any involved actor is blacklisted, let's say the user, delegateMulti will revert, obstructing him from reovering his tokens. Furthemore, if for any reason the proxy contract is blacklisted, the tokens will also be stuck in the contract.

• Users might be unable to interact with the contract if their address and/or the proxy is/are blacklisted.
• This issue is medium because the impact is high and likelihood is low.

Proof of Concept

1. A user interacts with the contract.
2. Their address is added to a blacklist, either through an off-chain process or another contract call.
3. Subsequent interactions from the blacklisted address are rejected. (same for the proxy address)

• Run the following test in a foundry environnement. Command : forge test --mt testDelegateMultiReimburseWithBlacklist -vvvv

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {console, Test} from "forge-std/Test.sol";

import "@openzeppelin/token/ERC20/ERC20.sol";
import "@openzeppelin/token/ERC20/extensions/draft-ERC20Permit.sol";
import "@openzeppelin/token/ERC20/extensions/ERC20Votes.sol";
import {ERC20MultiDelegate, ERC20ProxyDelegator} from "../src/ERC20MultiDelegate.sol";
import {StdCheats} from "forge-std/StdCheats.sol";

contract ERC20Mock is ERC20, ERC20Permit, ERC20Votes {
    constructor() ERC20("ERC20Mock", "E20M") ERC20Permit("Ethereum Name Service") {}

    function mint(address account, uint256 amount) external {
        _mint(account, amount);
    }

    function burn(address account, uint256 amount) external {
        _burn(account, amount);
    }

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

contract USDCMock is ERC20Mock {
    error USDCMock__AccountBlackListed();

    mapping(address => bool) public blacklisted;

    function blacklist(address _account) external {
        blacklisted[_account] = true;
    }

    function removeBlacklist(address _account) external {
        blacklisted[_account] = false;
    }

    modifier notBlacklisted(address _account) {
        require(!blacklisted[_account], "Blacklistable: account is blacklisted");
        _;
    }

    function transfer(address recipient, uint256 amount)
        public
        virtual
        override
        notBlacklisted(msg.sender)
        notBlacklisted(recipient)
        returns (bool)
    {
        return super.transfer(recipient, amount);
    }

    function transferFrom(address _from, address _to, uint256 amount)
        public
        virtual
        override
        notBlacklisted(_from)
        notBlacklisted(_to)
        returns (bool)
    {
        return super.transferFrom(_from, _to, amount);
    }
}

contract ERC20MultiDelegateTest is StdCheats, Test {
    ERC20MultiDelegate private multiDelegate;
    USDCMock private votesToken;
    address user1 = makeAddr("User1");
    address user2 = makeAddr("User2");

    address private source1 = makeAddr("source1");
    address private source2 = makeAddr("source2");

    address private target1 = makeAddr("target1");
    address private target2 = makeAddr("target2");
    address private target3 = makeAddr("target3");
    address private target4 = makeAddr("target4");
    address private target5 = makeAddr("target5");
    address private target6 = makeAddr("target6");

    uint256 private amount = 1e18;

    function setUp() public {
        votesToken = new USDCMock();
        votesToken.mint(address(user1), 10000 ether);
        votesToken.mint(address(user2), 10000 ether);

        multiDelegate = new ERC20MultiDelegate(votesToken, "http://localhost:8081");

        vm.prank(address(user1));
        votesToken.approve(address(multiDelegate), type(uint128).max);

        vm.prank(address(user2));
        votesToken.approve(address(multiDelegate), type(uint128).max);
    }

    function testDelegateMultiReimburseWithBlacklist() public {
        uint256[] memory sources = new uint256[](0);

        uint256[] memory targets = new uint256[](2);
        targets[0] = uint256(uint160(target1));
        targets[1] = uint256(uint160(target2));

        uint256[] memory amounts = new uint256[](2);
        amounts[0] = amount;
        amounts[1] = amount;

        // Instanciate the proxy contracts for the first time and delegate voting power
        vm.prank(address(user1));
        multiDelegate.delegateMulti(sources, targets, amounts);

        sources = new uint256[](2);
        sources[0] = uint256(uint160(target1));
        sources[1] = uint256(uint160(target2));

        targets = new uint256[](0);

        // Blacklisted user
        votesToken.blacklist(user1);
        vm.expectRevert(bytes("Blacklistable: account is blacklisted"));
        vm.prank(user1);
        multiDelegate.delegateMulti(sources, targets, amounts);

        // Blacklisted Proxy address

        address proxyAddress1;

        // Get proxy Address for first target
        bytes memory bytecode =
            abi.encodePacked(type(ERC20ProxyDelegator).creationCode, abi.encode(votesToken, target1));
        bytes32 hash = keccak256(
            abi.encodePacked(
                bytes1(0xff),
                address(multiDelegate),
                uint256(0), // salt
                keccak256(bytecode)
            )
        );
        proxyAddress1 = address(uint160(uint256(hash)));
        // Remove user from blacklist
        votesToken.removeBlacklist(user1);

        // only blacklist proxy address
        votesToken.blacklist(proxyAddress1);
        vm.expectRevert(bytes("Blacklistable: account is blacklisted"));
        vm.prank(user1);
        multiDelegate.delegateMulti(sources, targets, amounts);
    }
}

Tools Used

• Contract code review.
• Foundry testing

Recommended Mitigation Steps

• Consider introducing a functionality that allows users to specify what address they'd like to be paid, that way even a blocklisted account can specify a different address he/she owns.
• Allow the proxies to be updatable by a dedicated admin role to avoid transfer blacklisting.

Assessed type

Token-Transfer

[c4-pre-sort]

141345 marked the issue as duplicate of #216

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-a