ENS - lukejohn's results

QA1. function delegateMulti(): We should have zero address check for each address in targets and zero amount check for each value in amounts. Otherwise, we will assign zero address as the delegates.

[https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L57-L63C1])https://github.com/code-423n4/2023-10-ens/blob/ed25379c06e42c8218eb1e80e141412496950685/contracts/ERC20MultiDelegate.sol#L57-L63C1_

QA2. The constructor of ERC20MultiDelegate fails to pass arguments to its ancestor Ownable.

The mitigation is as follows:

  constructor(
        ERC20Votes _token,
        string memory _metadata_uri
    ) ERC1155(_metadata_uri) Ownable(msg.sender) {
        token = _token;
    }

QA3. The retrieveProxyContractAddress() is a private function used by the ERC20MultiDelegate, which always uses token. Therefore the argument of _token is not necessary.

function retrieveProxyContractAddress(
-        ERC20Votes _token,
        address _delegate
    ) private view returns (address) {
+       ERC20Votes _token = token;
        bytes memory bytecode = abi.encodePacked(
            type(ERC20ProxyDelegator).creationCode, 
            abi.encode(_token, _delegate)
        );
        bytes32 hash = keccak256(
            abi.encodePacked(
                bytes1(0xff),
                address(this),
                uint256(0), // salt
                keccak256(bytecode)
            )
        );
        return address(uint160(uint256(hash)));
    }

QA4. I suggest to split the delegateMulti() function into three functions: transferDelegation, reimburseDelegation; and createDelegation as follows. It not only saves gas (due to elimination of many comparisons), but also improve UX (they can pick which function to call based on each of the three cases).

    function transferDelegation(
        uint256[] calldata sources,
        uint256[] calldata targets,
        uint256[] calldata amounts
    ) external {
        uint256 sourcesLength = sources.length;
        uint256 targetsLength = targets.length;
        uint256 amountsLength = amounts.length;

        require(sourcesLength > 0, "empty array");
        require(sourcesLength == targetsLength && targetsLength == amountsLength, "array lengths not equal.");

        for (uint i; i < sourcesLength; i++)  _processDelegation(address(uint160(sources[i])), address(uint160(targets[i])), amounts[i]);

        _burnBatch(msg.sender, sources, amounts);
        _mintBatch(msg.sender, targets, amounts, "");
    }

    function reimburseDelegation(
        uint256[] calldata sources,
        uint256[] calldata amounts
    ) external {
        uint256 sourcesLength = sources.length;
        uint256 amountsLength = amounts.length;

        require(sourcesLength > 0, "empty array");
        require(sourcesLength ==  amountsLength, "array lengths not equal.");

        for (uint i; i < sourcesLength; i++)  _reimburse(address(uint160(sources[i])), amounts[i]);

        _burnBatch(msg.sender, sources, amounts);
    }

    function createDelegation(
        uint256[] calldata targets,
        uint256[] calldata amounts
    ) external {
        uint256 targetsLength = targets.length;
        uint256 amountsLength = amounts.length;

        require(targetsLength > 0, "empty array");
        require(targetsLength == amountsLength, "array lengths not equal.");

        for (uint i; i < targetsLength; i++)  createProxyDelegatorAndTransfer(address(uint160(targets[i])), amounts[i]);

        _mintBatch(msg.sender, targets, amounts, "");
    }

QA5. I suggest to allow ERC20ProxyDelegator() to delegate any ERC20 tokens not just for a fixed one:

contract ERC20ProxyDelegator {
    address owner;
    address delegate;
     
    constructor(address _delegate) {
        owner = msg.sender;
        delegate = _delegate;
    }

    function delegateToken(ERC20 _token) public {
        require(msg.sender == owner);

        _token.approve(msg.sender, type(uint256).max);
        _token.delegate(_delegate);

}

G1. Math.max(sourcesLength, targetsLength) should only be evaluated once. The second time we can use amountsLength since we have already checked they are equal earlier.

 function _delegateMulti(
        uint256[] calldata sources,
        uint256[] calldata targets,
        uint256[] calldata amounts
    ) internal {
        uint256 sourcesLength = sources.length;
        uint256 targetsLength = targets.length;
        uint256 amountsLength = amounts.length;

        require(
            sourcesLength > 0 || targetsLength > 0,
            "Delegate: You should provide at least one source or one target delegate"
        );

        require(
            Math.max(sourcesLength, targetsLength) == amountsLength,
            "Delegate: The number of amounts must be equal to the greater of the number of sources or targets"
        );

        // Iterate until all source and target delegates have been processed.
        for (
            uint transferIndex = 0;
-            transferIndex < Math.max(sourcesLength, targetsLength);
+            transferIndex < amountsLength;

            transferIndex++
        ) {
            address source = transferIndex < sourcesLength
                ? address(uint160(sources[transferIndex]))
                : address(0);
            address target = transferIndex < targetsLength
                ? address(uint160(targets[transferIndex]))
                : address(0);
            uint256 amount = amounts[transferIndex];

            if (transferIndex < Math.min(sourcesLength, targetsLength)) {
                // Process the delegation transfer between the current source and target delegate pair.
                _processDelegation(source, target, amount);
            } else if (transferIndex < sourcesLength) {
                // Handle any remaining source amounts after the transfer process.
                _reimburse(source, amount);
            } else if (transferIndex < targetsLength) {
                // Handle any remaining target amounts after the transfer process.
                createProxyDelegatorAndTransfer(target, amount);
            }
        }

        if (sourcesLength > 0) {
            _burnBatch(msg.sender, sources, amounts[:sourcesLength]);
        }
        if (targetsLength > 0) {
            _mintBatch(msg.sender, targets, amounts[:targetsLength], "");
        }
    }

G2. Math.min(sourcesLength, targetsLength) should be evaluated only once, not each time in the iteration.

 function _delegateMulti(
        uint256[] calldata sources,
        uint256[] calldata targets,
        uint256[] calldata amounts
    ) internal {
        uint256 sourcesLength = sources.length;
        uint256 targetsLength = targets.length;
        uint256 amountsLength = amounts.length;

+       uint minLen = Math.min(sourcesLength, targetsLength);

        require(
            sourcesLength > 0 || targetsLength > 0,
            "Delegate: You should provide at least one source or one target delegate"
        );

        require(
            Math.max(sourcesLength, targetsLength) == amountsLength,
            "Delegate: The number of amounts must be equal to the greater of the number of sources or targets"
        );

        // Iterate until all source and target delegates have been processed.
        for (
            uint transferIndex = 0;
-            transferIndex < Math.max(sourcesLength, targetsLength);
+            transferIndex < amountsLength;

            transferIndex++
        ) {
            address source = transferIndex < sourcesLength
                ? address(uint160(sources[transferIndex]))
                : address(0);
            address target = transferIndex < targetsLength
                ? address(uint160(targets[transferIndex]))
                : address(0);
            uint256 amount = amounts[transferIndex];

-            if (transferIndex < Math.min(sourcesLength, targetsLength)) {
+            if (transferIndex < minLen)) {

                // Process the delegation transfer between the current source and target delegate pair.
                _processDelegation(source, target, amount);
            } else if (transferIndex < sourcesLength) {
                // Handle any remaining source amounts after the transfer process.
                _reimburse(source, amount);
            } else if (transferIndex < targetsLength) {
                // Handle any remaining target amounts after the transfer process.
                createProxyDelegatorAndTransfer(target, amount);
            }
        }

        if (sourcesLength > 0) {
            _burnBatch(msg.sender, sources, amounts[:sourcesLength]);
        }
        if (targetsLength > 0) {
            _mintBatch(msg.sender, targets, amounts[:targetsLength], "");
        }
    }

G3. ERC20MultiDelegate._processDelegation() calls transferBetweenDelegators(), which will calculate the proxy address again for target although it has been returned by deployProxyDelegatorIfNeeded(). So we can save gas by implementing the transfer directly here:

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

        assert(amount <= balance);

+       address proxyAddressFrom = retrieveProxyContractAddress(token, from);

-        deployProxyDelegatorIfNeeded(target);
+       address proxyAddressTo = deployProxyDelegatorIfNeeded(target);
       
-       transferBetweenDelegators(source, target, amount);

+       token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);

        emit DelegationProcessed(source, target, amount);
    }

G3. Checking the proxy address of a given address is really expensive. It is better to introduce a mapping called proxyAddresses, we reimplement the following functions to save gas:

+  mapping(address => address) proxyAddresses;

   function deployProxyDelegatorIfNeeded(
        address delegate
    ) public returns (address proxy) {
         proxy = proxyAddresses[delegate];

         if(proxy == address(0))  
            proxy = address(new ERC20ProxyDelegator{salt: 0}(token, delegate));
            emit ProxyDeployed(delegate, proxyAddress);
        }
    }

 function transferBetweenDelegators(
        address from,
        address to,
        uint256 amount
    ) internal {
        address proxyAddressFrom = proxyAddresses[from];
        address proxyAddressTo = proxyAddresses[to];
        require(proxyAddressFrom != address(0) && proxyAddressTo != address(0);
        token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);
    }

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);
+        address proxyAddressFrom = proxyAddresses[source];

        token.transferFrom(proxyAddressFrom, msg.sender, amount);
    }

G4 To save gas, for function _delegateMulti(), there is no need to calculate source and target, which is a waste of gas due to comparison of transferIndex < sourcesLength and transferIndex < targetsLength. These two comparisons can be eliminated.

 function _delegateMulti(
        uint256[] calldata sources,
        uint256[] calldata targets,
        uint256[] calldata amounts
    ) internal {
        uint256 sourcesLength = sources.length;
        uint256 targetsLength = targets.length;
        uint256 amountsLength = amounts.length;

        require(
            sourcesLength > 0 || targetsLength > 0,
            "Delegate: You should provide at least one source or one target delegate"
        );

        require(
            Math.max(sourcesLength, targetsLength) == amountsLength,
            "Delegate: The number of amounts must be equal to the greater of the number of sources or targets"
        );

        // Iterate until all source and target delegates have been processed.
        for (
            uint transferIndex = 0;
            transferIndex < Math.max(sourcesLength, targetsLength);
            transferIndex++
        ) {
-            address source = transferIndex < sourcesLength
-                ? address(uint160(sources[transferIndex]))
-                : address(0);
-            address target = transferIndex < targetsLength
-                ? address(uint160(targets[transferIndex]))
-                : address(0);
-            uint256 amount = amounts[transferIndex];

            if (transferIndex < Math.min(sourcesLength, targetsLength)) {
                // Process the delegation transfer between the current source and target delegate pair.
-                _processDelegation(source, target, amount);
+                _processDelegation(sources[transferIndex], targets[transferIndex], amount); 
            } else if (transferIndex < sourcesLength) {
                // Handle any remaining source amounts after the transfer process.
-                _reimburse(source, amount);
+                _reimburse(sources[transferIndex], amount);

            } else if (transferIndex < targetsLength) {
                // Handle any remaining target amounts after the transfer process.
-                createProxyDelegatorAndTransfer(target, amount);
+                createProxyDelegatorAndTransfer(targets[transferIndex], amount);

            }
        }

        if (sourcesLength > 0) {
            _burnBatch(msg.sender, sources, amounts[:sourcesLength]);
        }
        if (targetsLength > 0) {
            _mintBatch(msg.sender, targets, amounts[:targetsLength], "");
        }
    }

G5. For for function _delegateMulti(), the comparisons of transferIndex < Math.min(sourcesLength, targetsLength, transferIndex < sourcesLength, and transferIndex < targetsLength is a waste of gas since they will be conducted for each iteration. In the following, we differentiate the three cases outside of the loop and thus save gas by not comparing for each iteration. We only need to compare once. in addition, we move the _burnBatch statement to the beginning to save some gas since when _burnBatch fail, there is no sufficient balance for the batch transfer, there is no need to execute the rest.

   function _delegateMulti(
        uint256[] calldata sources,
        uint256[] calldata targets,
        uint256[] calldata amounts
    ) internal {
        uint256 sourcesLength = sources.length;
        uint256 targetsLength = targets.length;
        uint256 amountsLength = amounts.length;

        if (sourcesLength > 0) {
            _burnBatch(msg.sender, sources, amounts[:sourcesLength]);
        }

        require(
            sourcesLength > 0 || targetsLength > 0,
            "Delegate: You should provide at least one source or one target delegate"
        );

        require(
            Math.max(sourcesLength, targetsLength) == amountsLength,
            "Delegate: The number of amounts must be equal to the greater of the number of sources or targets"
        );

        if(sourcesLength == targetsLength){
           for(int i; i < sourcesLength; i++) _processDelegation(sources[i], targets[i], amounts[i]);
        }
        else if(sourcesLength < targetsLength){
           for(int i; i < sourcesLength; i++) _processDelegation(sources[i], targets[i], amounts[i]);
           for(int i = sourcesLength; i < targetsLength; i++) {
                createProxyDelegatorAndTransfer(targets[i], amount);
           } 
       }
       else{
          for(int i; i < targetsLength; i++) _processDelegation(sources[i], targets[i], amounts[i]);
          for(int i = targetsLength; i < sourcesLength; i++) {
                _reimburse(sources[i], amount);
           }          
       }

        if (targetsLength > 0) {
            _mintBatch(msg.sender, targets, amounts[:targetsLength], "");
        }
    }

G6. Each call of retrieveProxyContractAddress(_token, _delegate) can be replaced by a call of deployProxyDelegatorIfNeeded() that is implemented as follows. Therefore, the function retrieveProxyContractAddress(_token, _delegate) can be deleted!

+  mapping(address => address) proxyAddresses;

   function deployProxyDelegatorIfNeeded(
        address delegate
    ) public returns (address proxy) {
         proxy = proxyAddresses[delegate];

         if(proxy == address(0))  
            proxy = address(new ERC20ProxyDelegator{salt: 0}(token, delegate));
            emit ProxyDeployed(delegate, proxyAddress);
        }
    }

G7. There is no need to check the balance in function _processDelegation(). Since delegateMulti() will burn ERC1155 tokens, which is equivalent to check balances. Therefore, the check of balance in _processDelegation() can be deleted to save gas:

    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);
    }