Skip to content
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

Fixes from Austin's test feedback on apply process #90

Merged
merged 5 commits into from
Mar 22, 2024

Conversation

Pabl0cks
Copy link
Member

Description

Tackling what we could consider bugs, or stuff that could confuse the user during his grant application process.

Leaving last item from #85 for another PR, since we could consider it more like an improvement/extra feedback: show a link of the TXs sent (approved / completed). <ins>50 % sent TX</ins>

  • Let sign /apply message from any network.
    "We tried to sign the /apply message and it failed (we were connected on Mainnet, and it said something like "wrong network" switch to OP). We should allow to sign on any of the two."
  • Remove askAmount from the signature message on /apply
  • /my-grants should hide the amount when the status is "proposed". Only admins need to see that on /admin (0.25 as default... they will be able to edit when Let admins edit grants info at the admin panel (only proposed status) #83 is done)

If you prefer to split in different PRs some of those items, or add the last item from the feedback in this PR, let me know! 🙌

Copy link

vercel bot commented Mar 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
grants-bg ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 22, 2024 4:10pm

@carletex
Copy link
Contributor

carletex commented Mar 22, 2024

Thank you @Pabl0cks !! Everything looks good!

Just pushed 346ad39 to avoid some possible security implications (which is I not a big deal in this case): since we are not signing the askAmount anymore, you could do a replay attack => send the same exact signature but change the askAmount (1000 ETH :D) in the POST payload => the backend will swallow it since we are not validating the askAmount on the signature.

So now we are just hardcoding it on the backend.

@carletex carletex merged commit 6d17bc3 into main Mar 22, 2024
3 checks passed
@carletex carletex deleted the fix/austin-test-feedback branch March 22, 2024 16:14
@carletex carletex mentioned this pull request Mar 22, 2024
4 tasks
@Pabl0cks
Copy link
Member Author

Just pushed 346ad39 to avoid some possible security implications (which is I not a big deal in this case): since we are not signing the askAmount anymore, you could do a replay attack => send the same exact signature but change the askAmount (1000 ETH :D) in the POST payload => the backend will swallow it since we are not validating the askAmount on the signature.

Ohh dang! Good to learn about it, TYSM for the catch and detailed explanation! 🙌❤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants