Governor can rug pull the escrow

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/gateway/BridgeEscrow.sol#L28-L30

Vulnerability details

Impact

Governor can rug pull all GRT held by BridgeEscrow, which is a severe undermining of decentralization.

Proof of Concept

The governor can approve an arbitrary address to spend any amount from BridgeEscrow, so they can steal all escrowed tokens. Even if the governor is well intended, the contract can still be called out which would degrade the reputation of the protocol (e.g. see here: https://twitter.com/RugDocIO/status/1411732108029181960). This is especially an issue as the escrowed tokens are never burnt, so the users would need to trust the governor perpetually (not about stealing their L2 tokens, but about not taking a massive amount of L1 tokens for free for themselves).

This seems an unnecessary power granted to the governor and turns a decentralized bridge into a needless bottleneck of centralization.

Tools Used

Code inspection

Recommended Mitigation Steps

Restrict access to approveAll() to the "bridge that manages the GRT funds held by the escrow". Or, similarly to how finalizeInboundTransfer in the gateways is restricted to its respective counterpart, only allow spending via other protocol functions.

[trust1995]

This is well documented in their spec sheet. They want the approveAll() functionality to save the escrow from all sorts of catastrophic failures.

[0xean]

dupe of #40

[trust1995]

I don't quite understand how a risk explicitly pointed out in the reference proposal, that requires Governor permissions, can be promoted to a M. I understand C4 has a commitment to users who view our audit report, but:

1. It is unfair to wardens who go by the book and don't submit issues that are intentional design choices.
2. It can still be included in the Low severity category.
3. It is not on par with the severity of other M submissions.

I believe we should have an open discussion on how to treat centralization risks, which is why I opened an org discussion here

[0xean]

Yup. I agree. I think it’s a bad practice as well.

On Oct 21, 2022, at 4:43 PM, Trust @.***> wrote:

 I don't quite understand how a risk explicitly pointed out in the reference proposal, that requires Governor permissions, can be promoted to a M. I understand C4 has a commitment to users who view our audit report, but:

It is unfair to wardens who go by the book and don't submit issues that are intentional design choices. It can still be included in the Low severity category. It is not on par with the severity of other M submissions. I believe we should have an open discussion on how to treat centralization risks, which is why I opened an org discussion here

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you modified the open/close state.

[trust1995]

From the fairness and validity in C4 docs: "Code4 has no role in determining the outcomes of findings and does not put its hand on the scale in individual contests." "What constitutes 'valid' report?" "The validity of an audit report submission is not based on whether it is ‘true’ or not. A report may contain a finding which is factually 'true' (the most literal interpretation of 'valid'), but if it does not add value or if it is not presented in such a way that adds value to a sponsor, it may be deemed invalid by a judge."

Judges are free to award findings in the way they see fit. We can agree this submission is just re-stating an intentional design choice of TheGraph to be able to approve another account in case of emergency or compromise. It is unfair to treat this finding, which is a known issue, as the same severity as novel attacks. See above for my reasoning.