TOB-WORM-4: Panicking on invalid inputs in Solana contracts

[tbjump]

trafficstars

Severity: Informational

Description

In several places within the solana smart contracts, the code panics when an arithmetic overflow/underflow occurs. Panics should be reserved for programer errors, e.g., assertion violations. Panicking on user errors dilutes panics’ utility.

Exploit Scenario

Alice, a Wormhole developer, observes a panic in the Wormhole codebase. Alice ignores the panic believing it is caused by user error, but it is actually caused by a bug she introduced.

Recommendations

Short term, reserve panics for programmer errors. Return Result::Err for user errors. Adopting such a policy will help to distinguish the two types of errors when they occur.

Long term, consider denying the following Clippy lints: ● clippy::expect_used ● clippy::unwrap_used ● clippy::panic Doing so will not prevent all panics, but it will prevent many of them.

[kii-dot]

Are there repro steps? Interested to take this on.

[tbjump]

Hi, for more details please see the audit report from ToB, item TOB-WORM-4 https://storage.googleapis.com/wormhole-audits/Wormhole_Audit_Report_TrailOfBits_2022-09.pdf

[kii-dot]

When I'm building the solana package, I'm getting

error: environment variable `BRIDGE_ADDRESS` not defined
   --> bridge/program/src/accounts/posted_vaa.rs:102:46
    |
102 |         AccountOwner::Other(Pubkey::from_str(env!("BRIDGE_ADDRESS")).unwrap())
    |                                              ^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in the macro `env` (in Nightly builds, run with -Z macro-backtrace for more info)

error: environment variable `EMITTER_ADDRESS` not defined
  --> bridge/program/src/api/governance.rs:47:28
   |
47 |     let expected_emitter = std::env!("EMITTER_ADDRESS");

Are there dummy address that I can use?

[kii-dot]

#1672

[kii-dot]

I believe this can be closed now since #1672 has been merged

[evan-gray]

I think some of the cases in the cosmwasm token bridge may have been missed due to changes that occurred after you started your PR. Leaving this open until someone confirms. Feel free to take a look as well @kii-dot 🙂

[kii-dot]

sounds good. I'll work on that. If there are more rust work do tag me :) @evan-gray

[tbjump]

Hey @kii-dot @evan-gray, are you still working on it?

For this issue to be considered fixed, the clippy lints should be enabled in CI. Anyone want to take a stab at that?

[kii-dot]

Since I've been working on it, let me take it up.

[kii-dot]

hey @tbjump @evan-gray Due to my schedule, I wont be able to attack this for the current time being. :(

[ameya-deshmukh]

@aadam-10 is this open to take up?

[aadam-10]

hi @ameya-deshmukh, please feel free to take a shot at this.

[aadam-10]

hi @ameya-deshmukh, do you have any updates on this?

[ameya-deshmukh]

Hey @aadam-10! Give me till the weekend to open a PR for this?

[aadam-10]

hi @ameya-deshmukh, wondering if you'd had a chance to look into this. it's not urgent or anything, but someone else may want to pick it up.

[ameya-deshmukh]

hi @aadam-10 yes. I'm actually working on a related project being built on Wormhole and this is actually one of the few issues which seemed relevant to be solved. I'll be opening a PR soon. Cheers