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

[Climber] Fix logic bug for the schedule delay not being considered #16

Closed
wants to merge 2 commits into from

Conversation

0237h
Copy link

@0237h 0237h commented Jul 8, 2022

Hi tincho !
I'm currently delving into the smart contract security world and your work on the damn-vulnerable-defi challenges has been a immense source of knowledge for me to learn about those exploits so thanks a lot for that !

I recently completed the Climber challenge and I was wondering how the ClimberTimelock let me execute immediately after calling schedule since there should be a 1 hour delay and I did not change it through the updateDelay function.

I figured there is a little logic bug in the getOperationState function : it should check if the block.timestamp is >= to the op.readyAtTimestamp, not the other way around.

This effectively affects the resolution of this challenge :

Spoiler I solved the challenge by granting my contract the "proposer" role to be able to schedule any transaction from the `ClimberTimelock` contract (full solution here). When first calling `execute` to set the role, there is now an additional step of calling `updateDelay` first to set the schedule delay to zero to be able to finish the `execute` transaction (or else it will simply revert).

I didn't change the challenge setup but I could also include tests for ensuring the delay is being considered if needed.

Thanks for reviewing this !

@tinchoabbate
Copy link
Collaborator

Hey, thanks for your suggestion. This may have been an unintended bug :) Perhaps it's solved in V3, perhaps it's not. We'll see!

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

Successfully merging this pull request may close these issues.

2 participants