Comprehensive Defense Against Arbitrary Minting not properly implemented as claimed by the whitepaper.

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/node/zetaclient/zeta_supply_checker.go#L113-L116 https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/protocol-contracts/contracts/evm/ZetaConnector.non-eth.sol#L16 https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/protocol-contracts/contracts/evm/ZetaConnector.non-eth.sol#L69

Vulnerability details

Description

The whitepaper claim at section 7.3 that the protocol implement a defense mechanism to prevent ZETA token to be inflated with invalid mint, such that the total supply cannot be inflated in case of bugs being exploited.

However, such mechanism is currently only partially implemented which seems to warrant Medium severity.

Impact

Documentation claim is inaccurate and the Zeta platform cannot claim to have such Comprehensive Defense Against Arbitrary Minting mechanism in place in the moment.

Proof of Concept

I would like to describe in more details what are the current weakness of the defense mechanism.

1. As indicated in another issue I've submitted titled ZetaSupplyChecker could potentially cause Observer to panic and also erroneous calculate the totalspply, ZetaSupplyChecker:
   • Can be inaccurate in his totalSupply calculation due to a bug
   • Is purelly passive in the moment as CheckZetaTokenSupply is only printing a log in case of error and ValidateZetaSupply return value (so if totalSupply is good or not) is not even consummed.
   • When the Observer starts, in case NewZetaSupplyChecker fails, the code execution will print a log and continue as is, making this feature a nice-to-have, but not mandatory (as it should be).

     zetaSupplyChecker, err := mc.NewZetaSupplyChecker(cfg, zetaBridge, masterLogger)
     if err != nil {
     startLogger.Err(err).Msg("NewZetaSupplyChecker")
     }
     if err == nil {
     zetaSupplyChecker.Start()
     defer zetaSupplyChecker.Stop()
     }

2. Supply check in connector contract is weak.

   • ZetaConnectorNonEth::onReceive is only checking against the totalSupply of the current external chain, not accounting for all the other supply available on other chains (which ZetaSupplyChecker try todo)

   • maxSupply is initialized with maxSupply = 2 ** 256 - 1; maximum uint256 value at contract deployment, and might not be set to a lower value later on (using setMaxSupply, and what would be this magic value?), which make this check useless if the maxSupply has not be overwritten afterwards. And I just checked on now (December 8th, 11h41 UTC-5) the Mumbai's connector, and confirm that this is the case in the moment, maxSupply has not been overwritten yet (so still at 115792089237316195423570985008687907853269984665640564039457584007913129639935, which expose this flaw.

     function onReceive(
     bytes calldata zetaTxSenderAddress,
     uint256 sourceChainId,
     address destinationAddress,
     uint256 zetaValue,
     bytes calldata message,
     bytes32 internalSendHash
     ) external override onlyTssAddress {
     if (zetaValue + ZetaNonEthInterface(zetaToken).totalSupply() > maxSupply) revert ExceedsMaxSupply(maxSupply);
     ZetaNonEthInterface(zetaToken).mint(destinationAddress, zetaValue, internalSendHash);
     
     if (message.length > 0) {
         ZetaReceiver(destinationAddress).onZetaMessage(
             ZetaInterfaces.ZetaMessage(zetaTxSenderAddress, sourceChainId, destinationAddress, zetaValue, message)
         );
     }
     
     emit ZetaReceived(zetaTxSenderAddress, sourceChainId, destinationAddress, zetaValue, message, internalSendHash);
     }

Recommended Mitigation Steps

Remove such claim from the WhitePaper or complete the mechanism such that it work as described. If you want to complete it, I would suggest the following steps:

1. Fix the 3 weaknesses I described for ZetaSupplyChecker.
2. Fix the 2 weaknesses I described for ZetaConnectorNonEth.sol. I would remove the maxSupply logic from the connector contract as a whole, right now it doesn't bring any value, and put all this inside the Observer code logic, such that before calling onReceive (SignOutboundTx) or onRevert (SignRevertTx), you add a check that communicate with ZetaSupplyChecker to see if this new supply can be minted, as this is the only place where you have the full picture of the totalsupply.

Assessed type

Error

[DadeKuma]

Possible divergence from whitepaper, probably QA

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-a

[dontonka]

My report is exposing a clear divergence with the ZetaChain whitepaper and the implementation (ccing: @lumtis as I don't think he saw the report since classified as insufficient quality report and should be aware). The fact that this security mechanism is taking up an entire section in their whitepaper, but more importantly that it is supposed to protect all the ZETA holders of a potential bug that would allow to invalid mint of ZETA, combined with the fact that the code in their smart contract as shown in my PoC is kind of weak (see Supply check in connector contract is weak), all this combined seems to warrant at least Medium severity.

Why does this matter? The negative impact explained in the whitepaper would be possible: an unknown vulnaribility being exploited to allow invalid minting that inflates the total supply of ZETA across chains. MintZetaToEVMAccount which is used here is the central point to minting ZETA and is not connected at all with the ZetaSupplyChecker::ValidateZetaSupply, which seems to be the only piece of logic in the ZetaChain node that do some sort of calculation in that regards. Furthermore, the whitepaper claim that The total supply of ZETA is provided by Chainlink and posted on each connected chain, which seems innacurate, at least I could not find any code that is doing such thing. Again, only ZetaSupplyChecker seems to be implemented in the moment, but is acting in a silo and as a passive tool.

For all the mention above, I would ask @0xean to reconsider this issue to be upgraded to Medium and unique finding and I would also appreciate the feedback of the sponsor @lumtis on the report. I saw multiple contest in 2023 where documentation divergence with implementation did warrant Medium and this seems to be one of them, it's not a simple bleep on the radar, it's an important security mecanism for the ZetaChain which is not walking the talk.

[0xean]

https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#estimating-risk

If you want to argue severity you need to do within the framework. I will welcome one last comment on the topic, but tagging in the sponsor hoping for a different response doesn't help your case. A fact based response if welcomed, but arguing they should remove a section of the white paper is definitely QA. Your report shows potential "weaknesses" but no clear exploit.