Bugfix in signature validations marketplace #1320
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
Jipperism
approved these changes
May 25, 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 PR covers the bug discussed in #1317
When a user has set the approvals on the marketplace, an attacker could submit a transaction signed by the attacker for the (partial) sale of a hypercert fraction. The attacker would receive the funds, the order taker would receive the fraction and the fraction owner would lose a bit of the fraction.
To mitigate this, we added checks on the hypercert strategies where needed. Specifically, a check on ownership of the fraction by the signer of the message by calling the HyperMinter contract at time of order execution.
The vulnerability is specific to the split function call because there is not check there on the operator-msg.sender relations/approval as common in the transferFrom methods. This is an artifact from changing the original design where only the owner or somebody allowed by the owner would be able to split. The marketplace widens the attack vector because anybody can operate the marketplace, compared to a trusted operator you specifically set the approval for.
To validate the changes, tests have been added to each hypercert strategy and one on the protocol level for 721 as a sanity check.