009.md

dinesh

medium

Using the low-level function ".delegatecall" or ".call" doesn't checks for contract existence

Summary

Using the low-level function ".delegatecall" doesn't checks for contract existence

Vulnerability Detail

At https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts/contracts/libraries/resolver/Lib_ResolvedDelegateProxy.sol#L51 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts/contracts/L1/messaging/L1StandardBridge.sol#L225 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts/contracts/L1/messaging/L1CrossDomainMessenger.sol#L203 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-periphery/contracts/universal/Transactor.sol#L31

    fallback() external payable {
        address target = addressManager[address(this)].getAddress(
            (implementationName[address(this)])
        );

        require(target != address(0), "Target address must be initialized.");

        // slither-disable-next-line controlled-delegatecall
        (bool success, bytes memory returndata) = target.delegatecall(msg.data);

        if (success == true) {
            assembly {
                return(add(returndata, 0x20), mload(returndata))
            }
        } else {
            assembly {
                revert(add(returndata, 0x20), mload(returndata))
            }
        }
    }
}

Impact

This function does not check for the existence of the contract at the target address. The require statement only checks that the target address is not the address(0) which is a null address, but it does not check if there is an actual contract deployed at that address.

It is possible that there is no contract deployed at the target address, or that the contract has been self-destructed, so the delegatecall function would fail and the execution of the fallback function would stop.

and at https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-periphery/contracts/universal/Transactor.sol#L31 Using .call instead of .transfer is generally considered to be safer because it allows the developer to check the return value of the call and ensure that the transfer was successful, and also allows to use a require() statement to check if the call to the target contract failed due to an error and avoid reentrancy issues.

Code Snippet

    fallback() external payable {
        address target = addressManager[address(this)].getAddress(
            (implementationName[address(this)])
        );

        require(target != address(0), "Target address must be initialized.");

        // slither-disable-next-line controlled-delegatecall
        (bool success, bytes memory returndata) = target.delegatecall(msg.data);

        if (success == true) {
            assembly {
                return(add(returndata, 0x20), mload(returndata))
            }
        } else {
            assembly {
                revert(add(returndata, 0x20), mload(returndata))
            }
        }
    }
}

Tool used

Manual Review

Recommendation

It is recommended to check the contract existence by using the low-level function extcodesize which returns the size of the code of a contract or 0 if the address is not a contract. Or you can use libraries like SafeMath or SafeERC20 that provide the functionality to check for contract existence before calling the delegatecall function.