code423n4 commented 1 year ago

Lines of code

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L308-L321

Vulnerability details

Proof of Concept

The function isContract() is used in the protocol to check whether the address is a contract, and revert if the address is not a contract.

From Openzeppelin,

It is unsafe to assume that an address for which this function returns false is an externally-owned account (EOA) and not a contract. Among others, isContract will return false for the following types of addresses:

an externally-owned account

a contract in construction

an address where a contract will be created

an address where a contract lived, but was destroyed

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/c12076fb7e3dfe48ef1d9c3bb2a58bdd3ffc0cee/contracts/utils/Address.sol#L18-L24

    function setUpgradeExecRouteBuilder(UpgradeExecRouteBuilder _router)
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
    {
        _setUpgradeExecRouteBuilder(_router);
    }

    function _setUpgradeExecRouteBuilder(UpgradeExecRouteBuilder _router) internal {
        address routerAddress = address(_router);

        if (!Address.isContract(routerAddress)) {
            revert NotAContract({account: routerAddress});
        }

Impact

Contract address check can be bypassed.

Tools Used

Manual Review

Recommended Mitigation Steps

Note the security issues of isContract() and find another way to mitigate it.

Assessed type

Context

0xSorryNotSorry commented 1 year ago

Technically correct, since no funds are in risk, could be QA

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as low quality report

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)