Pi/pool-manage-fix #81
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The output_index field doesn't refer to the index within the list of inputs, but the index within the outputs on the tx that produced it. So this code would have only worked sometimes, coincidentally, and maybe even have introduced subtle exploits. Instead, we scan over the inputs to find the pool input being spent, so we can make sure the redeemer correctly points to that one.
Aiken only includes types it can see from the transitive dependency of the top level; There were a few types that were unused at the surface, so didn't appear in the final blueprint. This just adds a `documentation` validator to make sure they show up
When withdrawing the last bit of liquidity, we were checking that there were 3 or 4 tokens on the output; but because we withdrew the last bit, that isn't sufficient
rrruko
approved these changes
May 6, 2024
This is actually really subtly important; we were checking that the *details* were signed, which excludes the validity range and the txRef; that would have allowed someone to repurpose one signature for a different order. It's important that the signature covers the whole execution, not just the details.
Quantumplation
force-pushed
the
pi/pool-manage-fix
branch
from
da36f90 to
d301422
Compare
ignaciodopazo
approved these changes
May 7, 2024
Quantumplation
commented
May 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This primarily fixes a few last minute bug/vulnerability we found during mainnet testing.
First, the pool manage code misunderstood what the
output_indexwould refer to. It was treated as if it was the index within the inputs of the pool output, but it was in fact just the normal output_index from the producing transaction.To fix this, we just scan over the inputs to find the index of the pool input.
Second, we can't withdraw all of the liquidity, because "pool_has_correct_output" checks for 3 or 4 tokens. To fix this, we just do a different check if we have 0 LP tokens left.
Finally, strategies were signing the details, rather than the whole execution. This could have led to reusing one signature for a different order, or using the signature outside of the intended validity window.
In addition, since we're making changes anyway, this does some other non-functional cleanup: