Fix permissionless middleware approving by whitelisting middleware programs in propose. #222
Conversation
This file contains hidden or 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
rado0x54
force-pushed
the
dev-rado0x54/bugfix/FYEO-CRY-02-protect-approve-middleware
branch
from
6a13438 to
c2a64fc
Compare
It was discovered that a permission middleware approval process, while NOT allowing transactions by an unauthorized signer, would however allow to BLOCK an authorized signer from execution. This PR introduces a new state variable on transaction account that whitelists all middleware programs that can approve middleware. Furthermore, this PR removes a legacy slot parameter.
rado0x54
force-pushed
the
dev-rado0x54/bugfix/FYEO-CRY-02-protect-approve-middleware
branch
from
c2a64fc to
8fafeff
Compare
tbarri
approved these changes
Feb 23, 2023
tbarri
reviewed
Feb 23, 2023
| pub approved_middleware: Option<Pubkey>, | ||
| /// The slot in which the transaction was proposed | ||
| /// This is used to prevent replay attacks | ||
| pub slot: u8, | ||
| /// The transaction state, to prevent replay attacks |
There was a problem hiding this comment.
Super minor comment, and for next time.
Furthermore, this PR removes a legacy slot parameter.
This minor change should be in a separate Git commit. We should maintain 1 idea = 1 commit.
If we wanted to rollback either the permission fix or the slot change, we can't because the changes are entangled. I'm certain we won't be doing this - but the point remains. Separate commits = easier code review + easier reverts. For next time! :-)
Contributor
Author
There was a problem hiding this comment.
Haha, yes and should I tell you something. It actually was a separate commit, but I applied the squashing to liberal :(. Should have paid a litte more attention.
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.
PERMISSIONLESS APPROVEEXECUTION INSTRUCTION ALLOWS ATTACKERS TO BLOCK ANOTHER USER'S EXECUTETRANSACTION INSTRUCTION
A permissionless instruction is when anyone on the Solana network can execute a transaction that interacts with the user’s programs. In some cases, permissionless instructions can be harmless depending on the logic of the program in question. In the case of Cryptid, the permissionless ApproveExecution instruction can have negative effects on the functionality of core operations, such as the ExecuteTransaction instruction.
This PR fixes the discovered issue.