ENS - Shogoki'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

The contract is using the ERC20 transferFrom function to transfer tokens to/from the user to the proxy or in between different proxy contracts. However, as the return value is not checked it might fail silently for some Tokens

This can finally lead to an vulnerability where an attacker can steal all the delegated Tokens.

Classifying this as a Medium, as these Tokens are quite rare, but still the sponsor mentioned that the contract should work with any Token that complies to the ERC20Votes interface.

Proof of Concept

Every time the delegateMulti function is called by a user, the contract is transferring amounts of the underlying ERC20 token. For this the ERC20 transferFrom function is used. According to the ERC20 specification, this function has to return an boolean to indicate it´s success. Most of the implementations also revert on a failed transfer, this is the case for the ENSToken, too. However, not all implementations will revert all the time, therefore the return value of the transferFrom call should be checked to make sure the transfer did not fail. This is not done inside the ERC20MultiDelegate contract, therefore the risk of a silently failing transfer exists. This is a problem because it can result in a loss of funds.

Example:

A malicious user has no Voting Tokens, but knows that the transferFrom on this token does not revert on any failure, but returns false. The user also knows that alice has delegated 10 Tokens to Bob already.

• The attacker now calls delegateMulti([],[bob],[10]).
• The contract will try to transfer 10 Tokens from the attacker to bobProxy, which will fail, as he has no Tokens. However as the return value is not checked the function will continue and mint 10 ERC1155 Tokens with id(bob) to the attacker.
• The attacker now calls delegateMulti([bob],[],[10]) to reimbures the ERC1155 Tokens for the voting Tokens.
• This time the transfer from bobProxy to the attacker will succeed, as the proxy already holds tokens from alice.
• The 10 ERC1155 Tokens from the attacker will be burned.

--> As a result the attacker was able to steal Alice´s Tokens

Example2:

Alice wants to delegate 10 Tokens to Bob and calls delegateMulti([],[bob],[10]) to achieve this. The contract will do the following:

• create the proxy for Bob if it does not exist
• call transferFrom(alice,proxyBob,10) (We imagine this transfer to work as expected)
• mints 10 ERC1155 tokens with id(bob) to alice

Now Alice wants to undelegate 5 of these tokens from Bob and get them back. Therefore she calls delegateMulti([bob],[],[5]) Now the contract is doing the following:

• calling transferFrom(proxyBob,alice,5) (this time this transfer silently fails)
• burns 5 ERC1155 tokens with id(bob) from alice

Because of the silently failed transfer, Alice would end up with no Tokens returned and only 5 ERC1155 Tokens left (Lost 5 Tokens)

Tools Used

Manual review

• nvim
• hardhat

Recommended Mitigation Steps

The return value of the transferFrom calls should be checked!

This could be done, but not has to, by using the SafERC20 Library from Openzeppelin.

Assessed type

Token-Transfer