-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix: implement proper authorization checks #2555
base: staging
Are you sure you want to change the base?
Conversation
Thank you for reporting!!! In the future, please use Hackerone: https://hackerone.com/aleo or the noted email address: https://github.com/AleoNet/snarkVM/blob/mainnet-staging/SECURITY.md I assume the design (unauthorized calls) was made on purpose, because it allows for withdrawing without having to make the cold withdrawal address warm. The risk of an adversary timing well enough and executing this function is low, given that CC issue created by the author: #2556 |
Thank you for your prompt response and for providing the proper channels for reporting security issues. While I understand and agree with the convenience of allowing cold withdrawal addresses to operate without becoming warm, I'd like to respectfully highlight some concerns regarding transaction traceability and blockchain explorer functionality:
|
Yes agree the UX should be good! Which explorer did you use? Maybe you can file an issue there, adding specific URLs to explain which data you think you'd be helpful if it would be showcased. |
I used several influential explorers, but none of them could track the transition of the withdrawal address. I once studied the code of aleo open source explorer. The transition data is obtained from the block data. If the blockchain metadata does not provide the functions I mentioned above, then this data will not be tracked. Explorers: Reference code: |
This seems like a UI issue and not related to snarkVM. To address your points specifically:
To verify my claims, try to curl a It will return the following. Notice the first input has the address of the staker's address:
|
I've conducted a similar test using the method you mentioned. Upon examining the results of the Here's the breakdown of the transaction I've referenced:
I try to curl a
However, the returned data from the API call does not include the initiator's or the withdrawal address It seems that it is understandable that the transaction signer will not appear. If I am the recipient of the withdrawn funds, my record will not be found in the entire blockchain. |
I don't believe the address of the initiator of the transaction is important. This is a permissionless call & the identity of the initiator is not important as it is already possible to pay fees in Aleo privately. Restricting the initiator to a particular address, in this case the withdrawal address would force validators to use a very sensitive key more often. Given that the only purpose of this is to made make indexing easier, it does not seem wise to compromise security in this way. In Aleo in particular, because you can pay fees privately, the initiator is hidden in many cases. Also even if the fee is public, there are fee paying services one can use to have someone else generate the fee. As for the concern about being about to see the withdraw address, you can already index this from Explorers in Aleo are already indexing all mapping changes. Here's an example from AleoScan: https://testnet.aleoscan.io/transaction?id=at18g44t5eerdwxh8chzd3ydvuxpqfqlvlmsgqsg7m98h3z6craqgyqan553l If you click on the mapping operations tab near the bottom you will see: I believe that will give you the data you want. This explorer is also open-source: https://github.com/HarukaMa/aleo-explorer |
Yes, we can parse FinalizeOperation from the specific transaction, such as RemoveKeyValue and UpdateKeyValue. However, if the tag information is missing in the contract, the transition will not be able to fully track the change of funds. From the perspective of account funds, if the explorer wants to obtain the correct fund data of a certain user, it needs to track all the records of the user's Address changing in the mapping, because all fund change information cannot be fully represented by transition, and the essence of each amount change is to operate the account mapping in mappings. |
This is actually a tracking issue that is relatively hard for explorers.
Detailed mapping value changes in Aleo network are not really visible unless you are able to directly tap into the finalize block execution during block advances; the
As Aleo credits operations can only happen in In your example, the withdraw address is
We can see that The transition didn't appear in the "Recent transitions" tab, and it's indeed because the withdraw address is not immediately visible in inputs and outputs of the transition. However, as shown above, this is just a design quirk of the current UI. There is a plan to add a new token flow tracking tab specifically for Aleo credits and tokens that adheres to ARC-21 standard in the next iteration of AleoScan UI.
Also see the point above; as the
This is almost true for AleoScan. Unfortunately, due to how post ratify works, currently AleoScan is not tracking full per-operation history of Edit: (TL;DR: Footnotes
|
Regarding the PR itself, directly changing the |
@HarukaMa Hey there! thanks for getting back to me. I've gone through the code, and sure enough, it was tracking the changes in the Mapping. I'm really excited about your upcoming Aleoscan UI design. |
ux is fine |
Hi all! Thanks for the fruitful discussion. I would recommend closing this PR and moving discussion to an ARC or other Aleo forum. Changes to the |
Motivation
This PR addresses a critical security vulnerability in the
claim_unbond_public
method of thecredits.aleo
contract. The issue potentially allows unauthorized addresses to claim unbonded funds. and incomplete transaction records, potentially affecting the accuracy of explorer statistics and the ability to track fund movements effectively. For a comprehensive explanation of the vulnerability, including its potential impact and suggested fixes, please refer to the detailed issue description. Your careful review and prompt attention to this matter are greatly appreciated.Test Plan
Test Plan for changes in synthesizer/process/src/tests/test_credits.rs:
Related PRs
(Link your related PRs here)