search

code-423n4 / 2024-03-taiko-findings

3 stars 2 forks source link

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

Closed c4-bot-6 closed 4 months ago

c4-bot-6 commented 5 months ago

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

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 commented 5 months ago

minhquanym marked the issue as primary issue

c4-pre-sort commented 5 months ago

minhquanym marked the issue as sufficient quality report

dantaik commented 5 months ago

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 commented 5 months ago

dantaik (sponsor) confirmed

0xean commented 5 months ago

@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 commented 5 months ago

0xean marked the issue as satisfactory

0xean commented 5 months ago

@dantaik - see https://github.com/code-423n4/2024-03-taiko-findings/issues/205 for perhaps a better impact statement

c4-judge commented 4 months ago

0xean marked the issue as selected for report

mcgrathcoutinho commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

c4-judge commented 4 months ago

0xean marked the issue as not selected for report