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

0 stars 0 forks source link

Governor can rug all GRT by setting the gateway to her wallet (Governor may be hacked) #251

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/l2/token/L2GraphToken.sol#L59-L63 https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/l2/token/L2GraphToken.sol#L80-L83 https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/l2/gateway/L2GraphTokenGateway.sol#L137-L175

Vulnerability details

Impact

Governor can rug all GRT by setting the gateway to her wallet (Governor may be hacked).

Proof of Concept

First, the Governor set the gateway contract to her wallet

    function setGateway(address _gw) external onlyGovernor {
        require(_gw != address(0), "INVALID_GATEWAY");
        gateway = _gw;
        emit GatewaySet(gateway);
    }

Second, the Governor calls bridgeMint to mint GRT to her own wallet.

    function bridgeMint(address _account, uint256 _amount) external override onlyGateway {
        _mint(_account, _amount);
        emit BridgeMinted(_account, _amount);
    }

Next, the Governor set the gateway contract back to the correct one.

Finally, the Governor bridged all funds to L1 and dump all GRT for ETH

    function outboundTransfer(
        address _l1Token,
        address _to,
        uint256 _amount,
        uint256, // unused on L2
        uint256, // unused on L2
        bytes calldata _data
    ) public payable override notPaused nonReentrant returns (bytes memory) {
        require(_l1Token == l1GRT, "TOKEN_NOT_GRT");
        require(_amount > 0, "INVALID_ZERO_AMOUNT");
        require(msg.value == 0, "INVALID_NONZERO_VALUE");
        require(_to != address(0), "INVALID_DESTINATION");

        OutboundCalldata memory outboundCalldata;

        (outboundCalldata.from, outboundCalldata.extraData) = parseOutboundData(_data);
        require(outboundCalldata.extraData.length == 0, "CALL_HOOK_DATA_NOT_ALLOWED");

        // from needs to approve this contract to burn the amount first
        L2GraphToken(calculateL2TokenAddress(l1GRT)).bridgeBurn(outboundCalldata.from, _amount);

        uint256 id = sendTxToL1(
            0,
            outboundCalldata.from,
            l1Counterpart,
            getOutboundCalldata(
                _l1Token,
                outboundCalldata.from,
                _to,
                _amount,
                outboundCalldata.extraData
            )
        );

        // we don't need to track exitNums (b/c we have no fast exits) so we always use 0
        emit WithdrawalInitiated(_l1Token, outboundCalldata.from, _to, id, 0, _amount);

        return abi.encode(id);
    }

You have written that Does it use a timelock function? No

This means Governor can rug all funds in a matter of minutes. If the governor's wallet gets hacked such as due to using Profanity, all funds will be hacked.

Recommended Mitigation Steps

Timelock must be enforced. If enforced, you can pause the contract on the L1 side before funds have been stolen.

0xean commented 1 year ago

Sponsor is already aware of this attack

The current bridge design assumes no tokens are minted in L2 (other than through the bridge), so any submissions showing how an attacker can mint tokens in L2 (without compromising the governor account) would be particularly valuable.

Closing as invalid.