ENS - nirlin's results

Lines of code

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

Vulnerability details

Impact

NOTE: In bot report this was added into disputed issue due to reasoning where the bot assumed that the this contract will only wrap the ens token which does not return false on failure. But as the sponsor confirmed this contract should wrap and work with any token

Bot Race Disputed Issue

The return value is not checked for the safeTransferFrom that could lead to following problems if the token being used does not revert on failure and instead returns the false. Transfer can fail due to any reason, lack of funds, contract like USDT is paused and is such cases following scenerios may happen:

1. If the user is transferring the tokens from source proxy to target proxy, funds may not be transferred but nfts will be minted and burnt leading to unclaimaible assets in one proxy and can be claimed from other proxy even though nothing was transferred leading to loss of funds for the users.

2. If the user is getting the reimburse and the transferfFrom fails, nothing is transferred and nfts are burned so tokens remainned stuck in the contract.

3. If the user is transferring to the proxy, transferFrom may fail, transfer never happen and nfts get minted to user leading to user be able to steal funds in future with those nfts from the proxies.

Proof of Concept

First look at the following code snippet from the delegateMulti function, that calls three main functions where the transfers happen:

   if (transferIndex < Math.min(sourcesLength, targetsLength)) {
                // require(source != target, "Delegate: Source and target must be different");
                // Process the delegation transfer between the current source and target delegate pair.
                _processDelegation(source, target, amount);

            // if source is 1 than this can happen as there is no target, so no target means that we will reimburse the amount to the source.
            } else if (transferIndex < sourcesLength) {
                // Handle any remaining source amounts after the transfer process.
                _reimburse(source, amount);

            // if there is no source but there is a target, then we will create a proxy delegator and transfer the amount to the target
            } else if (transferIndex < targetsLength) {
                // Handle any remaining target amounts after the transfer process.
                createProxyDelegatorAndTransfer(target, amount);
            }

So now lets have a look at the indivisual functions, firstly reimburse as it is simple and easy to understand So if we have a source and no target relative to it, it means user wants to get the amount back from the proxy address derived from the source passed:

    function _reimburse(address source, uint256 amount) internal {
        address proxyAddressFrom = retrieveProxyContractAddress(token, source);
        token.transferFrom(proxyAddressFrom, msg.sender, amount);
    }

Lets say the transferFrom here fails :

So than on the following line nfts will be burnt as the sourceLength is > 0

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

So essentially, nothing is transferred back to the msg.sender but his NFTs are burnt so the funds are permanently locked in the proxy now.

Not lets have a look at the _processDelegation function:

    function _processDelegation(
        address source,
        address target,
        uint256 amount
    ) internal {
        uint256 balance = getBalanceForDelegate(source);

        assert(amount <= balance);

        deployProxyDelegatorIfNeeded(target);
        transferBetweenDelegators(source, target, amount);

        emit DelegationProcessed(source, target, amount);
    }

This function after making the necessary checks call the transferBetweenDelegators which does the following:

    function transferBetweenDelegators(
        address from,
        address to,
        uint256 amount
    ) internal {

        address proxyAddressFrom = retrieveProxyContractAddress(token, from);
        address proxyAddressTo = retrieveProxyContractAddress(token, to);
        token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);
    }

So it derives the proxy address and transfer between them, so if transferFrom fails here following will happen:

1. Nfts for the sources will be burnt for the amounts
2. Nfts will be minted for the targets for the amounts.

Which essentially means from the target more can be claimed than there actually is, and the left in sources can be claimed no more. Loss of funds for the last mover.

Third scenerio is where there is no source only target for a particular amount, in that case createProxyDelegatorAndTransfer is called which essentially transfers the tokens from the msg.sender into proxy address derived from the target address:

    function createProxyDelegatorAndTransfer(
        address target,
        uint256 amount
    ) internal {
        address proxyAddress = deployProxyDelegatorIfNeeded(target);
        token.transferFrom(msg.sender, proxyAddress, amount);
    }

If the transferFrom fails here, nothing will be transferred into proxy, but the nfts will be minted to msg.sender on the following line:

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

So now the user can claim the tokens from the proxy if it is already deployed and some one else have delegated to the same target already. With these nfts those tokens be stolen.

Tools Used

Manual review

Recommended Mitigation Steps

Use the safeErc20 library.

Assessed type

ERC20