Protocol does not check inside GuardianProver::approve() if all the guardians are approving the same proof

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/L1/provers/GuardianProver.sol#L33

Vulnerability details

Summary

The natspec for GuardianProver::approve() says the following on L33 but the "sent the same proof" clause is never really checked inside the function:

  File: contracts/L1/provers/GuardianProver.sol

  29:               /// @dev Called by guardians to approve a guardian proof
  30:               /// @param _meta The block's metadata.
  31:               /// @param _tran The valid transition.
  32:               /// @param _proof The tier proof.
  33:  @--->        /// @return approved_ If the minimum number of participants sent the same proof, and proving
  34:               /// transaction is fired away returns true, false otherwise.
  35:               function approve(
  36:                   TaikoData.BlockMetadata calldata _meta,
  37:                   TaikoData.Transition calldata _tran,
  38:                   TaikoData.TierProof calldata _proof
  39:               )
  40:                   external
  41:                   whenNotPaused
  42:                   nonReentrant
  43:                   returns (bool approved_)
  44:               {
  45:                   if (_proof.tier != LibTiers.TIER_GUARDIAN) revert INVALID_PROOF();
  46:                   bytes32 hash = keccak256(abi.encode(_meta, _tran));
  47:                   approved_ = approve(_meta.id, hash);
  48:           
  49:                   if (approved_) {
  50:                       deleteApproval(hash);
  51:  @--->                ITaikoL1(resolve("taiko", false)).proveBlock(_meta.id, abi.encode(_meta, _tran, _proof));
  52:                   }
  53:           
  54:                   emit GuardianApproval(msg.sender, _meta.id, _tran.blockHash, approved_);
  55:               }

Description & Impact

• Assume there are a total of 9 guardians. Hence approvals needed for approved_ = true is (9 + 1) / 2 = 5.
• Guardian1 to Guardian3 pass proofX while calling approve().
• Guardian4 passes proofY.
• Guardian5 passes proofX. Since the hash on L46 is calculated without involving the proof, L47 will return true at the 5th guardian's call even though Guardian4 had used proofY.
• proveBlock() is called with _proof equalling proofX.
• Note that whatever _proof Guradian5 uses while calling approve() will be used to call proveBlock() on L51, irrespective of whatever the previous guardians passed.

Thus the guardian proof is approved even when in reality the required votes have not been achieved.

Tools Used

Manual review

Recommended Mitigation Steps

Compare the _proof.data to make sure approval for the same proof is being provided by all the guardians.

Assessed type

Invalid Validation

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[dantaik]

I think this is a valid report, although the severity is not high. Fixed in this PR: https://github.com/taikoxyz/taiko-mono/pull/16606/files

[c4-sponsor]

dantaik (sponsor) confirmed

[0xean]

@dantaik - can you clarify why you don't believe this to be impactful? It does seem M is warranted as the function of the protocol is affected, no?

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[c4-judge]

0xean marked the issue as satisfactory

[0xean]

@dantaik - see #205 for perhaps a better impact statement

[c4-judge]

0xean marked the issue as selected for report

[mcgrathcoutinho]

Hi @0xean, I think this issue should be of QA-severity, here's why:

If there is no check present, the guardians sending another proof could be considered as incorrect input supplied by a trusted role. This falls under QA since trusted roles are providing incorrect parameters. Also to note, the guardians are selected by the DAO and Security council as Dan mentioned in his BCR video linked in the README.

Overall, I think the check is more of a sanity check than a major issue to the protocol's functionality.

Would like to hear your opinion on this in case I'm misjudging.

Thank you!

[0xean]

Hi @0xean, I think this issue should be of QA-severity, here's why:

If there is no check present, the guardians sending another proof could be considered as incorrect input supplied by a trusted role. This falls under QA since trusted roles are providing incorrect parameters. Also to note, the guardians are selected by the DAO and Security council as Dan mentioned in his BCR video linked in the README.

Overall, I think the check is more of a sanity check than a major issue to the protocol's functionality.

Would like to hear your opinion on this in case I'm misjudging.

Thank you!

See #205 -

The issue arises from the fact that the approved message doesn't include the _proof. It means that the last approving guardian can provide any desired value in the _proof. The data from the _proof is used to determine whether it is necessary to return the liveness bond to the assigned prover or not:

To me the above represents a privilege escalation or if nothing malicious an issue that could affect the functioning of the protocol. Based on that I think M is correct.

[c4-judge]

0xean marked issue #205 as primary and marked this issue as a duplicate of 205

[c4-judge]

0xean marked the issue as not selected for report