code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/l2/token/L2GraphToken.sol#L80-L93 https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/gateway/L1GraphTokenGateway.sol#L263-L279

Vulnerability details

Impact

Daily bridge amount limit should be enforced to prevent bridge takeover due to a single bug. For example, Nomad hack. Many bridges are being hacked nowadays.

Without a daily bridge amount limit, once a part for example Arbitrum sequencer is hacked, hackers can steal all funds in the bridge. Causing your long-built innovation to die due to this nonsense reason. Arbitrum is actively founding new vulnerabilities as days pass.

Pausing doesn't help anything as the hack happened in a matter of seconds, and you can't know when it will get hacked.

Proof of Concept

Assume the gateway is hacked

We can freely mint and burn unlimited GRT in L2

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

    /**
     * @dev Decreases token supply, only callable by the L1/L2 bridge (when tokens are transferred to L1).
     * @param _account Address from which to extract the tokens
     * @param _amount Number of tokens to burn
     */
    function bridgeBurn(address _account, uint256 _amount) external override onlyGateway {
        burnFrom(_account, _amount);
        emit BridgeBurned(_account, _amount);
    }

In L1, we can unlock all funds in one transaction too

    function finalizeInboundTransfer(
        address _l1Token,
        address _from,
        address _to,
        uint256 _amount,
        bytes calldata // _data, contains exitNum, unused by this contract
    ) external payable override notPaused onlyL2Counterpart {
        IGraphToken token = graphToken();
        require(_l1Token == address(token), "TOKEN_NOT_GRT");

        uint256 escrowBalance = token.balanceOf(escrow);
        // If the bridge doesn't have enough tokens, something's very wrong!
        require(_amount <= escrowBalance, "BRIDGE_OUT_OF_FUNDS");
        token.transferFrom(escrow, _to, _amount);

        emit WithdrawalFinalized(_l1Token, _from, _to, 0, _amount);
    }

Tools Used

Thinking of a way to prevent complete draining of remaining and future bridges

Recommended Mitigation Steps

Add a daily bridge amount limit to prevent bridge takeover due to a single bug. Governance should be able to set this parameter. With daily bridge amount enforced, once a hack occurs, only a limited amount of funds are being stolen.

0xean commented 2 years ago

Without POC showing how assets are at risk here and just a hypothetical attack, this is QA.