ENS - peakbolt's results

Lines of code

https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L148 https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L159-L161 https://github.com/code-423n4/2023-10-ens/blob/main/contracts/ERC20MultiDelegate.sol#L163-L171

Vulnerability details

ERC20MultiDelegate uses transferFrom() to move the ERC20 tokens in/out of the proxy contracts (see code below).

However, it fails to check the return value of transferFrom(). It is documented in EIP20 that callers must handle a false return value and must not assume false is never returned.

Some tokens such as ZRX and EURS do not revert in failure and instead returns false on failure.

Due to the missing check, the transferFrom() could fail silently and continue to mint/burn the ERC20MultiDelegate ERC1155 token. That will cause token holders to incur lossses as they do not receive the ERC20 tokens on withdrawals.

https://github.com/code-423n4/2023-10-ens/blob/1adbe2cce191140657b8bccffab85103953bdccb/contracts/ERC20MultiDelegate.sol#L148

    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);
        //@audit missing check for return value from transferFrom()
        token.transferFrom(proxyAddressFrom, msg.sender, amount);
    }

Note that this finding was disputed in the bot report likely based on the assessment that the ENSToken is using OZ ERC20 that will revert on failure. However, sponsor has clarified that the scope extends beyond ENSToken with the remark "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."

Impact

Due to the issue, there will be a mismatch in the mapping of the ERC20MultiDelegate ERC1155 tokens to the ERC20 tokens.

Token holders/proxy contracts will not receive ERC20 tokens when the outgoing transfer from proxy contract fails. On the other hand, token holders/proxy contracts can incorrectly gain ERC20 tokens when the incoming transfer into proxy contract fails.

Proof of Concept

Imagine the scenario,

1. Alice calls delegateMulti() to delegate 1000 ENSToken to Bob.
2. The transferFrom() failed silently but Alice still received the ERC20MultiDelegate ERC1155 tokens even thought the 1000 ENSToken remains in her account.
3. Now Charlie calls delegateMulti() to delegate 2000 ENSToken to Bob and transferFrom() suceeded.
4. Alice now uses her ERC1155 tokens to call delegateMulti() and withdraw 1000 ENSToken and steal from Charlie.
5. Charlie now suffers a loss of 1000 ENSTokens as the proxy contract balance is 1000 ENSToken after Alice's withdrawal.

Recommended Mitigation Steps

Wrap the transferFrom() with require().

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

During deployment of ERC20ProxyDelegator, it calls _token.approve() to grant spending allowance for ERC20MultiDelegate contract (see below).

The issue is the missing check for return value of approve(), which is mandated by EIP20 to ensure that false return value are handled.

If the approve() fails without revert and only returns false, the ERC20ProxyDelegator will fail to grant allowance for ERC20MultiDelegate. That will be disastrous as ERC20MultiDelegate will not be able to transfer ERC20 tokens out of the proxy contracts, causing the ERC20 tokens to be permanently lock in it as there are no mechanism to reapprove the allowance.

contract ERC20ProxyDelegator {
    constructor(ERC20Votes _token, address _delegate) {
        _token.approve(msg.sender, type(uint256).max);
        _token.delegate(_delegate);
    }
}

Note that this finding was disputed in the bot report likely based on the assessment that the ENSToken is using OZ ERC20 that will revert on failure. However, sponsor has clarified that the scope extends beyond ENSToken with the remark "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."

Impact

The issue can cause ERC20 tokens in the proxy contracts to be permanently locked.

Proof of Concept

1. Alice calls delegateMulti() to delegate 1000 ENS Tokens to Bob.
2. The ERC20ProxyDelegator for Bob is deployed for the first time.
3. The approve() fails sliently but Alice's 1000 ENSTokens is still transferred into the proxy contract.
4. Now Alice is unable to withdraw her ENSTokens as ERC20MultiDelegate is not granted allowance due to the failed approve().

Recommended Mitigation Steps

Wrap approve() with require().

Assessed type

Invalid Validation