code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/upgrades/GraphProxyAdmin.sol#L30-L36 https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/upgrades/GraphProxyAdmin.sol#L46 https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/upgrades/GraphProxyAdmin.sol#L58

Vulnerability details

Impact

Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent (per EVM design).

Solidity documentation warns:

“The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.”

Proof of concept

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/upgrades/GraphProxyAdmin.sol#L30-L36

File: /contracts/upgrades/GraphProxyAdmin.sol
    function getProxyImplementation(IGraphProxy _proxy) public view returns (address) {
        // We need to manually run the static call since the getter cannot be flagged as view
        // bytes4(keccak256("implementation()")) == 0x5c60da1b
        (bool success, bytes memory returndata) = address(_proxy).staticcall(hex"5c60da1b");
        require(success);
        return abi.decode(returndata, (address));
    }

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/upgrades/GraphProxyAdmin.sol#L46

File: /contracts/upgrades/GraphProxyAdmin.sol
46:        (bool success, bytes memory returndata) = address(_proxy).staticcall(hex"396f7b23");

Tools

Manual review

Recommendation

Check for the account existence before perfoming the low level call

trust1995 commented 1 year ago

Is there a possible threat scenario that can arise ?

0xean commented 1 year ago

closing as invalid.

code-423n4 / 2022-10-thegraph-findings