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: remove onchain vulnerability in gather fuel (fuel can be stolen) #109

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

francolq
Copy link
Contributor

@francolq francolq commented Dec 9, 2024

In a gather fuel operation, current validation doesn't check that the amount of fuel provided by the pellet is equal to the amount of fuel gathered by the ship. So, all fuel from both the pellet and the ship can be extracted to a wallet. Example:
https://preview.cexplorer.io/tx/a53aa9ba30e6ce6168403765fd103d73db86ed1b207e950a3ac542dbcce951b1

For the same reason, it is also possible to add external fuel to any ship. Example: https://discord.com/channels/946071061567529010/1214742273871319040/1315704064620630066

In this PR I provide the fix by checking that the amounts in both redeemers match. Another possible solution is to remove the amounts in the redeemers and do the checks over the values directly.
I also provide a couple of tests for the fix.

Comment on lines +175 to +180

// gathered fuel amount must be equal to amount provided by pellet
let pellet_purpose = Spend(pellet_input.output_reference)
let pellet_redeemer: Data = Provide(amount)
expect pairs.get_all(redeemers, pellet_purpose) == [pellet_redeemer]

Copy link
Member

Choose a reason for hiding this comment

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

One of the requirements is to be able to gather less than the available pellet fuel. This is useful if you're reaching the max amount of fuel permitted by the ship.

Would this changes still support that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

@francolq francolq requested a review from scarmuega January 29, 2025 16:38
@franciscojoray franciscojoray requested review from franciscojoray and removed request for scarmuega January 29, 2025 17:17
@franciscojoray franciscojoray requested review from franciscojoray and removed request for franciscojoray January 29, 2025 17:27
@scarmuega scarmuega merged commit 95c1f2b into txpipe:main Jan 29, 2025
1 check failed
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.

3 participants