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.

Sign up for GitHub

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

Jump to bottom

The Sway compiler currently disallows read access to storage when the call is made within the fallback function. #6324

Open
IGI-111 opened this issue Jul 30, 2024 · 0 comments · May be fixed by #6573
Open

The Sway compiler currently disallows read access to storage when the call is made within the fallback function. #6324

IGI-111 opened this issue Jul 30, 2024 · 0 comments · May be fixed by #6573
Assignees
@esdrubal
Labels
audit-report Related to the audit report bug Something isn't working P: low

Comments

@IGI-111
Copy link
Contributor

IGI-111 commented Jul 30, 2024

From https://bugs.immunefi.com/dashboard/submission/32730

Brief/Intro

The Sway compiler's current state Reverts any transaction that performs read access to storage if the call is made within the fallback function.

Vulnerability Details

I adapted an existing collection of tests targetting fallback functionality (can found at https://github.com/FuelLabs/sway/tree/7b56ec734d4a4fda550313d448f7f20dba818b59/test/src/sdk-harness/test_projects/run_external_proxy and https://github.com/FuelLabs/sway/tree/7b56ec734d4a4fda550313d448f7f20dba818b59/test/src/sdk-harness/test_projects/run_external_target) to demonstrate an issue with the way compiler handles storage access in fallback function. While the compiler documentation suggests READ access to storage should be possible within fallback functions (since it's a common characteristic of any Sway function), the current state of the compiler prevents this. This difference between the documentation and the codebase's actual behavior raises doubts about whether fallback functions work as intended.

However, WRITE operations to storage within fallback functions are working as expected, indicating a potential inconsistency in how storage interactions are handled.

Impact Details

A malicious contract with a faulty fallback function could be deployed, deliberately hiding crucial storage access features within it. This could trick less technical users into thinking the contract is legitimate because it compiles successfully. This can potentially to griefing;
If contract A interacts with contract B and the interaction fails, causing contract B to revert, contract A may also revert as a consequence;
References

sway/docs/book/src/blockchain-development/calling_contracts.md

Line 174 in 7b56ec7

For all intents and purposes the fallback function is considered a contract method, which means that it has all the limitations that other contract methods have. As the fallback function signature, the function cannot have arguments, but they can return anything.

Proof of concept

Please see following gists: https://gist.github.com/0xZRA/c25f0ee9ef89d44adde573609c5553f3 https://gist.github.com/0xZRA/19a9079f97f60a5143115e76a0eb84a9 https://gist.github.com/0xZRA/18d8f25e6b3a97025de946868a230765 https://gist.github.com/0xZRA/6e1684f538ad85dff5a39b25bb54523f https://gist.github.com/0xZRA/8875643f14e88092ac72e400079fd849
@IGI-111 IGI-111 self-assigned this Jul 30, 2024
@IGI-111 IGI-111 added bug Something isn't working audit-report Related to the audit report labels Jul 30, 2024
@IGI-111 IGI-111 removed their assignment Jul 30, 2024
@IGI-111 IGI-111 added the P: low label Jul 30, 2024
esdrubal added a commit that referenced this issue Sep 20, 2024
@esdrubal
Proof that #6324 is not a bug.
0da96a6 
The reproduction presented in #6324 was missing loading the storage
configuration properly.

To do that we have to use the following in test/src/sdk-harness/test_projects/run_external_proxy_with_storage/mod.rs:
    StorageConfiguration::default().add_slot_overrides_from_file("test_projects/run_external_target_with_storage/out/release/run_external_target_with_storage-storage_slots.json").unwrap();

This commit changes the presented reproduction to load the storage slots
and the test runs without reverting.

Closes #6324
@esdrubal esdrubal linked a pull request Sep 20, 2024 that will close this issue
Draft
8 tasks
esdrubal added a commit that referenced this issue Sep 26, 2024
@esdrubal
Proof that #6324 is not a bug.
e7fdb8e 
The reproduction presented in #6324 was missing loading the storage
configuration properly.

To do that we have to use the following in test/src/sdk-harness/test_projects/run_external_proxy_with_storage/mod.rs:
    StorageConfiguration::default().add_slot_overrides_from_file("test_projects/run_external_target_with_storage/out/release/run_external_target_with_storage-storage_slots.json").unwrap();

This commit changes the presented reproduction to load the storage slots
and the test runs without reverting.

Closes #6324
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@esdrubal esdrubal

Labels
audit-report Related to the audit report bug Something isn't working P: low
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Proof that #6324 is not a bug. FuelLabs/sway
2 participants
@esdrubal @IGI-111