Platform: Code4rena
Start Date: 07/10/2022
Pot Size: $50,000 USDC
Total HM: 4
Participants: 62
Period: 5 days
Judge: 0xean
Total Solo HM: 2
Id: 169
League: ETH
Rank: 4/62
Findings: 1
Award: $2,278.81
π Selected for report: 1
π Solo Findings: 0
2278.8081 USDC - $2,278.81
Governor can rug pull all GRT held by BridgeEscrow, which is a severe undermining of decentralization.
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.
Code inspection
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.
#0 - trust1995
2022-10-12T20:38:55Z
This is well documented in their spec sheet. They want the approveAll() functionality to save the escrow from all sorts of catastrophic failures.
#1 - 0xean
2022-10-16T13:39:55Z
dupe of #40
#2 - trust1995
2022-10-21T22:43:23Z
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:
I believe we should have an open discussion on how to treat centralization risks, which is why I opened an org discussion here
#3 - 0xean
2022-10-21T22:45:49Z
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.
#4 - trust1995
2022-10-25T07:28:00Z
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.