PolyNetwork Bug Review And Patch Analysis

[This blog is published with the authorization from PolyNetwork team]

Given the opportunity to perform an informal security review of the Pull Request 12 (PR-12) of the eth-contracts repository, we summarize in the report our assessment to evaluate the design goal, expose potential security issues, and examine semantic inconsistencies, if any, in the given smart contract implementation. Our analysis shows that the given PR-12 implementation provides the much-needed whitelist feature (in EthCrossChainManager) that prevents the unwanted invocation of arbitrary destination contracts and their methods, hence fixing the critical vulnerability that was exploited in the the PolyNetwork incident on August 10, 2021.

  • Review Target: https://github.com/polynetwork/eth-contracts/pull/12/files

The deployed version has an issue in blindly trusting the messages mined in the Relay Chain and failing to thoroughly and properly sanitize the deserialized message content before carrying out cross-chain transactions. Undoubtedly, the incident is a well-executed exploitation of the above issue that originates from Ontology and propagates to BSC, Ethereum, Polygon, and Heco (Note the attempt on Heco is not successful as the related relayer does not behave exactly the same as others. The exact reason is out of scope of this review, but is also a ``TASTY FOOD FOR THE RESEARCHERS.’’).

To elaborate, we show below the related verifyHeaderAndExecuteTx() function. This function is designed to verify a given relay chain message (with the related header and the associated Merkle proof) and execute the intended cross-chain transaction on the destination chain, e.g., BSC, Ethereum, Polygon, etc.

Figure 1: EthCrossChainManager::verifyHeaderAndExecuteTx()

The issue stems from the insufficient validation on the deserialized toMerkleValue struct after the proper Merkle proof and signature verification. Specifically, its makeTxParam member field may contain well-crafted content: for example, the three struct sub-fields toContract, method, and args could be together misused to invoke a privileged call (putCurEpochConPubKeyBytes()) on EthCrossChainData to change the effective keepers. Notice that this privileged call is declared as an onlyOwner operation. However, it is now misused, in a manner similar to the traditional return-to-libc (ret2libc) attack, to take over the current keepers! To recap, we show below how a malicious cross-chain transaction may deviate from a normal one.

Figure 2: The Cross-Chain Transaction Comparison in PolyNetwork: Normal vs. Exploited
  • Issue Fixup: The PR-12 is proposed to address the above issue by
    implementing a much-needed whitelist feature. This whitelist feature in essence defines the list of administrator-approved contracts as well as the associated methods that are then applied to validate the above member fields, especially toContract and method, to thwart any manipulation. For extra precaution, we also make the suggestion to define a whitelist that may be allowed to call the crossChain() function to initiate a cross-chain transaction. After discussion, the team takes the suggestion and includes it a part of this PR-12.

    To conclude, the proposed PR-12 achieves the intended goal by fixing the loophole in the original implementation. Once merged, it is ready to be deployed to upgrade (and fix) the deployed version.

The PDF version of our review is available at the following link: https://github.com/peckshield/publications/blob/master/audit_reports/polynetwork_pr12_review.pdf



A Blockchain Security Company (https://peckshield.com)

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store