initialize function in L2GraphToken.sol, BridgeEscrow.sol, L2GraphTokenGateway.sol, L1GraphTokenGateway.sol can be invoked multiple times from the implementation contract.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/l2/gateway/L2GraphTokenGateway.sol#L87
https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/l2/token/L2GraphToken.sol#L48
https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/gateway/L1GraphTokenGateway.sol#L99
https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/gateway/BridgeEscrow.sol#L20

Vulnerability details

Impact

initialize function in L2GraphToken.sol, BridgeEscrow.sol, L2GraphTokenGateway.sol, L1GraphTokenGateway.sol

can be invoked multiple times from the implementation contract.

this means a compromised implementation can reinitialize the contract above and

become the owner to complete the privilege escalation then drain the user's fund.

Usually in Upgradeable contract, a initialize function is protected by the modifier

 initializer

to make sure the contract can only be initialized once.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

1. The implementation contract is compromised,

2. The attacker reinitialize the BridgeEscrow contract

    function initialize(address _controller) external onlyImpl {
        Managed._initialize(_controller);
    }

the onlyGovernor modifier's result depends on the controller because

    function _onlyGovernor() internal view {
        require(msg.sender == controller.getGovernor(), "Caller must be Controller governor");
    }

3. The attacker have the governor access to the BridgeEscrow,

4. The attack can call the approve function to approve malicious contract

     function approveAll(address _spender) external onlyGovernor {
        graphToken().approve(_spender, type(uint256).max);
    }

5. The attack can drain all the GRT token from the BridgeEscrow.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project use the modifier

 initializer

to protect the initialize function from being reinitiated

   function initialize(address _owner) external onlyImpl initializer  {

[0xean]

Sponsor - please also review #72 which is a duplicate but has a slightly different impact statement to the same underlying issue.

[pcarranzav]

The impact from #72 was discussed with a warden and I consider it valid and worth fixing.

The impact from this one requires the implementation to be compromised in a way that allows calling initialize(). Since all implementations use GraphUpgradeable, I don't see how that's possible without also compromising the GraphProxyAdmin and therefore compromising the governor, which would mean much bigger problems.

I still consider this risky enough that a Medium severity is reasonable, both in #72 and this one.

[trust1995]

Exploit of this bug requires ProxyAdmin compromise, and if attacker has that then there are many ways to severely damage the proxy. It's a good observation and should be fixed, but it seems to be a best practice issue.

[pcarranzav]

Fix PRd in graphprotocol/contracts#741

[0xean]

@trust1995 - makes reasonable points, but I think its still correct to award this as M.

[trust1995]

Would like to restate my strong opinion that this is not a vulnerability but a best practice issue. The only way to abuse the lack of "initializer" modifier is with ProxyAdmin calling the IMPL contract. But it is known ProxyAdmin can just change the IMPL contract to any it desires. The risk of ProxyAdmin being compromised is a generic centralization issue, applicable to every upgradeable structure and is far too much of a known risk to qualify as M.

As further evidence, 5 QA reports categorized the find as Low / QA. They also should be upgraded to M, or this report rightfully moved to QA / Low.