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

panic on unwrapping in type_check_trait_implementation #6336

Closed
IGI-111 opened this issue Jul 30, 2024 · 0 comments · Fixed by #6434
Closed

panic on unwrapping in type_check_trait_implementation #6336

IGI-111 opened this issue Jul 30, 2024 · 0 comments · Fixed by #6434
Assignees
Labels
audit-report Related to the audit report bug Something isn't working

Comments

@IGI-111
Copy link
Contributor

IGI-111 commented Jul 30, 2024

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

Brief/Intro

The identified bug is a panic in the Sway compiler's semantic analysis module, specifically in the implementation of trait declarations. This occurs due to an unexpected None value being unwrapped during the type checking of trait implementations. If exploited in production, this bug could cause compiler crashes when processing certain trait implementations, potentially preventing developers from compiling valid Sway code. The bug highlights a need for more robust error handling in the compiler's trait implementation processing.

Vulnerability Details

The bug is in type_check_trait_implementation at

vec![type_decl.ty.clone().unwrap().type_id],
and
vec![type_decl.ty.clone().unwrap().type_id],

if type_decl.ty.clone() is None, it will cause unexpected unwarp panic like:

thread 'main' panicked at sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs:817:47:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Possible fix

A possible fix is checking whether type_decl.ty is None .

...
            ImplItem::Type(decl_id) => {
                let type_decl = engines.pe().get_trait_type(decl_id);
                let mut type_decl = type_check_type_decl(
                    handler,
                    ctx.by_ref(),
                    &type_decl,
                    trait_name,
                    implementing_for,
                    is_contract,
                    &impld_item_refs,
                    &type_checklist,
                )
                .unwrap_or_else(|_| {
                    ty::TyTraitType::error(ctx.engines(), type_decl.as_ref().clone())
                });
            if type_decl.ty.is_none() {
                        return   ty::TyTraitType::error(ctx.engines(), type_decl.as_ref().clone()) }
...

Impact Details

While this bug doesn't directly put funds at risk, its potential to introduce vulnerabilities and disrupt the development process makes it a severe issue. The compiler is a critical component of the blockchain development stack, and its reliability is paramount for the security and success of the entire ecosystem. Addressing this vulnerability is crucial to maintain the integrity and trustworthiness of the Fuel platform.

Proof of concept

Step 1
forc new poc
Step 2
write minimized code to main.sw

script;
trait Trait{}
struct Struct0{}
impl Trait for Struct0{type u;const u=0;}

Step3
forc build
It return panic like:

thread 'main' panicked at sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs:817:47:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
@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 assigned esdrubal and unassigned IGI-111 Jul 30, 2024
esdrubal added a commit that referenced this issue Aug 19, 2024
This PR fixes panic on unwrapping by only perform the required behaviour
with the unwrapped value when it is available.

Fixes #6336
esdrubal added a commit that referenced this issue Aug 19, 2024
This PR fixes panic on unwrapping by only perform the required behaviour
with the unwrapped value when it is available.

Fixes #6336
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-report Related to the audit report bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants