claim spoofing fix #149

tgrecojs · 2024-12-16T21:04:47Z

No description provided.

…t to claim

…ng handled correctly when processed through updated offerHandler code

…itedness as generic interface for creating AsyncGenerator subscribtions (#150) This commit contains test for: - Handling bobs offer - Subscribing to bobs wallet - Verifying Alice's inability to make a second claim attempt

…xisting test code to properly evaluate assertions against updated makeClaimAirdropInvitation - added Observable factory function for watching AsyncGenerators - implemented logic for informing smart wallets of tribbles issuer (fixes issue where provisionSmartWallet fails when interfacing with a new issuer) - added import statements for various type definitions (a practice that should continue whenever faced with VSCode intellisense errors stemming from unknown types)

amessbee · 2024-12-23T04:13:03Z

@tgrecojs I am going to review this, hopefully, today but it looks good at first glance. Could you maybe add very basics details in readme regarding how to run this on local? Thanks.

tgrecojs · 2024-12-23T15:47:51Z

@tgrecojs I am going to review this, hopefully, today but it looks good at first glance. Could you maybe add very basics details in readme regarding how to run this on local? Thanks.

Yes, no problem @amessbee. I will be sure to do so today.

Additionally, if you find that you would benefit from a deeper understanding of the code, I'd be more than happy to walk you through it during a short Zoom call. Feel free to reach out to me, and we can schedule a time that suits us both.

amessbee · 2024-12-23T16:02:09Z

@tgrecojs I am going to review this, hopefully, today but it looks good at first glance. Could you maybe add very basics details in readme regarding how to run this on local? Thanks.

Yes, no problem @amessbee. I will be sure to do so today.

Additionally, if you find that you would benefit from a deeper understanding of the code, I'd be more than happy to walk you through it during a short Zoom call. Feel free to reach out to me, and we can schedule a time that suits us both.

Awesome - that would be great. I think the contract code looks fine to me. I may still want to go through the tests with you if we can find time.

contract/Makefile

amessbee · 2024-12-23T15:24:01Z

contract/src/airdrop.contract.js

+            await Promise.all(
+              Object.values(payment).map(pmtP =>
+                E.when(pmtP, pmt => E(depositFacet).receive(pmt)),
+              ),
+            );


I was wondering if there is a case of multiple payments to an address given that depositFacet receives payment before the corresponding address is added to the accountStore in case of failure somewhere in between?

This is a great call out. Could you provide me with an example of one such error?

I'm not necessarily worried about a failure happening, however your comments did introduce a new scenario to me in which a user makes subsequent offers to claim before this Promise has resolved. If the user is able to get passed the accountStore book keeping occurs, then they would seemingly be able to collect more than one payment.

This is all the more reason for updating the code so that the book keeping logic happens before any Payment is sent out.

amessbee · 2024-12-23T15:27:28Z

contract/src/airdrop.contract.js

            rearrange(
-              harden([
-                [tokenHolderSeat, claimSeat, { Tokens: paymentAmount }],
-                [claimSeat, tokenHolderSeat, { Fee: claimTokensFee }],
-              ]),
+              harden([[claimSeat, tokenHolderSeat, { Fee: claimTokensFee }]]),
            );


This probably none to very limit impact in the current context, but we are probably breaking offer-safety here by separating deposit of tokens from claiming Fee. WDYT?

We've opted to allow for the offer to occur in such a manner enforces offer-safety for the tokenHolderSeat. This is due to the nature of this specific offer as it uses depositFacets one one side of the offer, and Zoe seat rearrange for the other.

If this is an issue, then I think the fix would be to move the accountStore logic so that it occurs before the depositFacet is sent a payment, effectively stopping any instances in which a user manages to get passed the check on accountStore before the Promise.all payment resolves.

Does that make sense?

I've realized that a merge I conducted for adding this commit into the codebase got lost along the way. I'm going to address this today. I wanted to make note of it though as it relates to ensuring offer safety.

amessbee · 2024-12-23T15:52:35Z

contract/test/tribbles-airdrop/contract.test.js

+test.serial('makePauseContractInvitation', async t => {
+  const merkleRoot = merkleTreeAPI.generateMerkleRoot(
+    accounts.map(x => x.pubkey.key),


I see that all tests in this module are passing but if possible, can we refactor these two tests (makePauseContractInvitation and E2E test) - I kind of lost track halfway through :)

amessbee · 2024-12-23T15:58:49Z

ui/src/components/Orchestration/MakeOffer.tsx

-        }
-      };
+  const offerArgsValue = makeOfferArgs({
+    address: wallet.address,


Question: if we are not using address in the contract, why are we still sending it here or expecting in offerArgs - do we expect a future use case?

No, we're not expecting to use it ever. I didn't mean to include this file in the PR so sorry for the confusion.

…his commit contains code that was introduced to the codebase on a seperate branch**. This is meant to be as an acknowledgement of this undesired (yet somewhat necessary) process

…rrect shape

tgrecojs

@tgrecojs I am going to review this, hopefully, today but it looks good at first glance. Could you maybe add very basics details in readme regarding how to run this on local? Thanks.

Yes, no problem @amessbee. I will be sure to do so today.

Additionally, if you find that you would benefit from a deeper understanding of the code, I'd be more than happy to walk you through it during a short Zoom call. Feel free to reach out to me, and we can schedule a time that suits us both.

Awesome - that would be great. I think the contract code looks fine to me. I may still want to go through the tests with you if we can find time.

@amessbee, here is a link to schedule a meeting with me. If you're unable to find a time that suits you, please let me know. I'm more than happy to accommodate. :)

https://scheduler.zoom.us/t/s4gpdxv0/1hr

amessbee · 2024-12-27T04:45:18Z

Thanks @tgrecojs - I have requested a meeting on Monday if that works. It may be too late though, so I am available at the same time on Fri/Sat/Sun so do let me know if we can meet earlier. Thanks.

tgrecojs · 2024-12-27T09:31:44Z

Thank you for setting the meeting @amessbee.

Considering the timeline, it would be really helpful if we could move the session to Saturday at the same time (9AM EST). This change would allow me to address any remaining action items before Monday, putting us in a strong position to finalize the review process at the start of the week.

If this new timing works for you, please let me know, and I'll adjust the invite accordingly.

Thanks again!

amessbee · 2024-12-27T12:45:40Z

@tgrecojs yes that works for me, let's meet on Saturday.

* refactor(offerHandler): - moved offerHandler book keeping so that it takes place at the time when Payments are sent to claimant's depositFacets - removed instances where precomputed logic was being used once - removed address string from accountStore lookup message to eliminate unnecessary computation

- added test code demonstrating makePauseInvitation working as expected - removed instances of unnecessary duplicate tests assertions - renamed e2e.test.js to coreEval.test.js

tgrecojs · 2025-01-06T05:42:50Z

hey @amessbee - can you stamp this as all good now that you have context into the decision to take this approach, and the conversations that were conducted with Dean, as well as others, confirming the validity of this approach?

you may have noticed but everything you mentioned has been addressed. thanks for all the useful feedback btw 🙂

amessbee

Great work - everything looks good!

tgrecojs and others added 6 commits December 12, 2024 20:23

feat: codespace config

a0c1ead

feat(depositFacet-claim): working implementation of using depositFace…

d7d6d72

…t to claim

chore: added depositFacet.test.js file

a77c256

test(depositFacet): added test code for verifying that offers are bei…

6ea4f42

…ng handled correctly when processed through updated offerHandler code

tgrecojs requested a review from amessbee December 20, 2024 18:27

amessbee reviewed Dec 23, 2024

View reviewed changes

tgrecojs added 2 commits December 25, 2024 10:54

feat(offer-validation): added code for validating offerArgs data. **T…

5a59eca

…his commit contains code that was introduced to the codebase on a seperate branch**. This is meant to be as an acknowledgement of this undesired (yet somewhat necessary) process

test: updated makeOfferArgs function to produce an object with the co…

d27c0e9

…rrect shape

tgrecojs commented Dec 26, 2024

View reviewed changes

tgrecojs and others added 4 commits December 30, 2024 00:52

chore(linting): removed unused variables

f1ea11d

chore(linting): removed unused variables from airdrop.proposal.js

85c40ab

test:

26c15b5

- added test code demonstrating makePauseInvitation working as expected - removed instances of unnecessary duplicate tests assertions - renamed e2e.test.js to coreEval.test.js

tgrecojs mentioned this pull request Dec 30, 2024

Document deployment process #153

Closed

2 tasks

amessbee approved these changes Jan 6, 2025

View reviewed changes

tgrecojs self-assigned this Jan 6, 2025

tgrecojs linked an issue Jan 6, 2025 that may be closed by this pull request

claim spoofing #141

Closed

tgrecojs added this to the Tribbles Airdrop Mainnet Launch milestone Jan 6, 2025

tgrecojs merged commit a442ac0 into main Jan 6, 2025
1 check passed

This was referenced Jan 7, 2025

offerArgs not validated #142

Closed

contract audit fixes #145

Closed

tgrecojs mentioned this pull request Jan 7, 2025

claim spoofing #141

Closed

tgrecojs deleted the #141-patch branch January 8, 2025 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

claim spoofing fix #149

claim spoofing fix #149

tgrecojs commented Dec 16, 2024

amessbee commented Dec 23, 2024

tgrecojs commented Dec 23, 2024

amessbee commented Dec 23, 2024

amessbee Dec 23, 2024

tgrecojs Dec 23, 2024 •

edited

Loading

amessbee Dec 23, 2024

tgrecojs Dec 23, 2024

tgrecojs Dec 23, 2024

amessbee Dec 23, 2024

amessbee Dec 23, 2024

tgrecojs Dec 23, 2024

tgrecojs left a comment

amessbee commented Dec 27, 2024

tgrecojs commented Dec 27, 2024

amessbee commented Dec 27, 2024

tgrecojs commented Jan 6, 2025

amessbee left a comment

claim spoofing fix #149

claim spoofing fix #149

Conversation

tgrecojs commented Dec 16, 2024

amessbee commented Dec 23, 2024

tgrecojs commented Dec 23, 2024

amessbee commented Dec 23, 2024

amessbee Dec 23, 2024

Choose a reason for hiding this comment

tgrecojs Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

amessbee Dec 23, 2024

Choose a reason for hiding this comment

tgrecojs Dec 23, 2024

Choose a reason for hiding this comment

tgrecojs Dec 23, 2024

Choose a reason for hiding this comment

amessbee Dec 23, 2024

Choose a reason for hiding this comment

amessbee Dec 23, 2024

Choose a reason for hiding this comment

tgrecojs Dec 23, 2024

Choose a reason for hiding this comment

tgrecojs left a comment

Choose a reason for hiding this comment

amessbee commented Dec 27, 2024

tgrecojs commented Dec 27, 2024

amessbee commented Dec 27, 2024

tgrecojs commented Jan 6, 2025

amessbee left a comment

Choose a reason for hiding this comment

tgrecojs Dec 23, 2024 •

edited

Loading