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
• Review Period: August 13, 2021 — August 15, 2021
• Issue Description: PolyNetwork is a cross-chain interoperability bridge (that allows a variety of chains to flexibly interact with each other and transfer arbitrary data along with carrying out cross-chain transactions). Arguably one of the largest cross-chain protocols in terms of Total Value Locked (TVL) and liquidity, it has so far supported a number of chains, including Ethereum, Binance Smart Chain (BSC), Polygon, Heco, Ontology, etc. To facilitate the implementation, the protocol has designed a number of cooperative components (each with its own roles and responsibilities), such as Relay Chain, Off-Chain Relayer, Keeper, as well as various smart contracts deployed on supported blockchains. In the following, we mainly focus on the smart contracts deployed on Ethereum as reflected in our review target.

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.
• Disclaimer: This is an informal security review, not a full security audit, and it does not give any warranties on finding all possible security
  issues of the given smart contract(s), i.e., the evaluation result does not guarantee the nonexistence of any further findings of security issues.
  Furthermore, we always recommend proceeding with several independent full audits and a public bug bounty program to ensure the security of smart contract(s). Lastly, this security review report should not be used as investment advice.

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