token.transferFrom signaling failure by returning false may get funds stuck

[c4-submissions]

Lines of code

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

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.

There are two situations where not checking the return value of token.transferFrom may cause funds to get stuck.

The first one happens when trying to delegate from an Address A to an Address B using delegateMulti and token.transferFrom fails. This will cause an update of ERC20MultiDelegate.balanceOf(Adress, AddressA) and ERC20MultiDelegate.balanceOf(Adress, AddressB) without updating the token balance of the proxies associated with AddressA and AddressB and thus causing the funds to be stuck.

The second one happens when reimbursing a user of ERC20MultiDelegate and token.transferFrom fails causing the value of ERC20MultiDelegate.balanceOf(Address, AddressX) to decrease without updating the token balance of the user and the proxy associated with the address. This results in the amount of funds that would be reimbursed to get stuck.

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 blocks below to test/delegatemulti.js inside the describe block "ENS Multi Delegate"

describe("ENS Multi Delegate", () => {
...
it("token.transferFrom signaling failure by returning false may cause funds to get stuck #1", async () => {
      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 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 causes transferFrom to fail.
      token.setCanTransferFrom(false);

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

      token.setCanTransferFrom(true);

      await expect(
        multiDelegate.delegateMulti([alice], [], amounts)
      ).to.be.revertedWith("ERC20: transfer amount exceeds balance");
      await expect(
        multiDelegate.delegateMulti(delegates, [], amounts)
      ).to.be.revertedWith("ERC1155: burn amount exceeds balance");
    });

it("token.transferFrom signaling failure by returning false may cause funds to get stuck #2", async () => {
      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 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 causes transferFrom to fail.
      token.setCanTransferFrom(false);

      await multiDelegate.delegateMulti(delegates, [], amounts);
      //Since transfer didn't happen balance is still 0
      expect(await token.balanceOf(deployer)).to.eq(0);
      expect(await multiDelegate.balanceOf(deployer, delegates[0])).to.eq(0);

      token.setCanTransferFrom(true);

      await expect(
        multiDelegate.delegateMulti(delegates, [], amounts)
      ).to.be.revertedWith("ERC1155: burn amount exceeds balance");
    });
...
})

Tools Used

Manual Review

Recommended Mitigation Steps

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

Assessed type

ERC20

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #91

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as duplicate of #90

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-c