Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(protocol): fix an issue for ProverSet.proposeBlock #17521

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

davidtaikocha
Copy link
Member

No description provided.

Copy link

fix(protocol): fix an issue for ProverSet.proposeBlock

Generated at commit: 31e73b9b66b8f885b48490f1df9b1b677b5ff500

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
2
0
8
42
54
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@davidtaikocha davidtaikocha enabled auto-merge June 8, 2024 04:16
@davidtaikocha davidtaikocha added this pull request to the merge queue Jun 8, 2024
Merged via the queue into main with commit d3037ad Jun 8, 2024
4 checks passed
@davidtaikocha davidtaikocha deleted the fix-propose-prover-set branch June 8, 2024 05:12
@@ -83,7 +83,7 @@ contract ProverSet is EssentialContract, IERC1271 {
bytes calldata _txList
)
external
onlyProver
Copy link
Contributor

@Brechtpd Brechtpd Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need this check because of this:

if (params.hookCalls.length == 0) {
if (params.assignedProver != msg.sender) revert L1_NOT_SAME_ADDRESS();
_tko.transferFrom(msg.sender, address(this), _config.livenessBond);

So now anybody could propose a block through this contract, set no hooks and the assignedProver to the pool contract, and use the TAIKO in this contract as a bond for the proving no? And then the provers in this contract are forced to prove the block, which seems unwanted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They still need an enabled prover to sign their blob at first?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean this check:

&& ECDSA.recover(meta_.blobHash, params.signature) != msg.sender
? This check is only done when no blob is used, so then still a problem when a blob is used I think.

hmmm also does ECDSA.recover even support contracts? Looking at https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol it doesn't seem to call isValidSignature anywhere for contract addresses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right, we may also need to whitelist the proposer addresses in proverSet @dantaik

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants