-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adding K-V storage to conditions allowing to implement protection between conditions #173
Conversation
…ause upgradeability issues This reverts commit 288cc3e.
@@ -138,6 +138,14 @@ contract TransferNFT721Condition is Condition, ITransferNFT, ReentrancyGuardUpgr | |||
lockConditionState == ConditionStoreLibrary.ConditionState.Fulfilled, | |||
'LockCondition needs to be Fulfilled' | |||
); | |||
|
|||
// Check that nft receiver is the same enabled in the lock payment condition | |||
require( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this helps...
The scenario I was thinking is the following:
- TransferCondition contract is approved by LockPaymentCondition contract to transfer token
id
(not certain if this can even happen currently, but for the casenftOwner == _lockConditionAddress
it is needed) - Correct flow: a lock payment condition is created, and a transfer condition depending on that. The lock payment condition is fulfilled
- Attack flow: another pair of conditions is created with the attacker as receiver. Attacker fulfills the bogus lock payment condition, and then can fulfill the transfer condition as receiver.
One possible solution could be to set the receiver when the token is sent to _lockConditionAddress
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Not sure that all new checks are necessary now that only the nft owner can fulfill.
I'm gonna leave the Conditions Key-Value implementation there because it can be useful/necessary for some complex service agreements |
Description
To add a better protection to fulfillment of conditions dependant on other conditions, this PR adds a KV storage related with the Condition Ids. This allow to publish parameters in the fulfillment of conditions that can be used by other conditions to validate/control the access/fulfillment.
This is implemented in the
TransferNFT721
conditionIs this PR related with an open issue?
Related to Issue #
Types of changes
Checklist: