Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sovereign Audit remediations #359

Open
wants to merge 20 commits into
base: feature/ongoingPP
Choose a base branch
from

Conversation

ignasirv
Copy link
Contributor

@ignasirv ignasirv commented Nov 18, 2024

The following PR implements de remediations for the audit.
It also fixes a security issue found aposteriori of the audit:

  • At sovereign contracts, for the context of a sovereign chain that also has FEP (full execution proofs) there is the possibility to insert arbitrary GER, make claims, and restore previous GER and get the transition proved.
    To avoid this behavior, the following remediations have been implemented:
    • The address that can insert GER and the address that can remove GERs is different, opening the possibility to set zero address to remove GERs functionality, specialy designed for FEP chains
    • A function unsetMultipleClaimedBitmap to unset claims from the bitmap has been implemented -> IMPORTANT to audit with careful, it makes tricky bit operations. Can only be called by bridgeManager
    • Many functions from main bridge contract are being overwritten at sovereign contract removing some unnecessary checks int the sovereign context. This has been done to reduce bytecode length to 24kb and make de sc deployable
    • Function setSovereignTokenAddress has been removed for bytecode length performance improvement
  • Due to this new changes, the bytecode exceeds the limit for a smart contract to be deployed, so some functions from PolygonZkEVMBRidgeV2 has been overwritten. They have been overwritten without some checks only necessary for bridges that are being deployed on L1, which is not the case.
  • The scripts and tests have been updated accordingly

@ignasirv ignasirv force-pushed the feature/audit-remediations branch 3 times, most recently from 450cd7e to d4e4bad Compare November 22, 2024 11:21
@ignasirv ignasirv force-pushed the feature/audit-remediations branch from d4e4bad to eac43cd Compare November 22, 2024 12:48
@@ -16,6 +14,10 @@ contract GlobalExitRootManagerL2SovereignChain is
// globalExitRootUpdater address
address public globalExitRootUpdater;

// globalExitRootRemover address
// In case of initializing a chain with Full execution proofs, this address should be set to zero, otherwise, some malicious sequencer could insert invalid global exit roots, claim, go back and the execution would be correctly proved.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment might not be right?¿ we coudl have a timelock or multisig for it, ( could improve the comment with it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? I understood that in case of a FEP chain, this address has to be zero. Indeed, if it's not zero can be a multisig but what happens with the comment?

contracts/v2/sovereignChains/BridgeL2SovereignChain.sol Outdated Show resolved Hide resolved
@ignasirv ignasirv requested a review from invocamanman December 3, 2024 08:16
Copy link

sonarcloud bot commented Dec 12, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants