Fix 'informational' issues (audit recommendations)

[ebma]

There are some very minor things brought up in the audit that we might want to fix. The following checklist only includes the item ID; the description and details can be found by looking at the audit report.

• TYP-01 Confusing Variable Naming
• SYS-03 Unnecessary Variable
• STA-02 Lack of Validation for destination_adress on send_payment_to_address()
• STA-01 Code duplication
• SRW-01 Usage of hard-coded string
• SRC-01 Unused Methods and Storage
• PRO-01 Unhandled Error
• PAE-02 Usage of magic numbers
• PAE-01 Unnecessary Result<> Return Type
• LIT-04 Reduce using unwrap() and expect() in Production Codebase
• LIT-03 Panic can happen between correlated storage modifications
• LII-01 Inconsistent match expression
• LIE-04 Mismatch in variable name and pallet
• LIL-01 Dead Code in execute_replace()
• IMP-01 Same Behavior Defined for different conditions
• EXC-01 Missing information in logging message
• CLI-03 Incorrect error type thrown
• CLI-02 Typos
• CLI-01 Confusion function naming
• 4FC-05 Commented out code #417
• 4FC-04 Logic should be moved to a separate function - refactoring
• 4FC-03 Unused errors
• 4FC-02 Inconsistent comments

Since the changes are so minor, it's okay to include all of them in one branch and one Pull Request. But each of them should at least be addressed in a separate commit with a proper name, also mentioning the ID in the name so that it's easier to track the changes.

additional:

• GLOBAL-05 Unneceessary Off-Chain User Protection Mechanism
• LI5-02 TryFrom CurrencyId implementations Contain Repeated Code
• LIH-03 Values Length Not Validated in feed_values Function

[vadaynujra]

Hey team! Please add your planning poker estimate with Zenhub @b-yap @ebma @TorstenStueber

[b-yap]

EXC-01 - Missing information in logging message
was completed by #377

[b-yap]

4FC-04 Logic should be moved to a separate function - refactoring -> #388

[b-yap]

could not find the ff. in skyharbor.certik:

• LIL-01 Dead Code in execute_replace()
• LIT-03 Panic can happen between correlated storage modifications

[ebma]

Oh right, that's weird. I also can't find them to be honest 😅
@b-yap can you check and compare the informational issues that are listed on page 46 ff. to the informational issues that are listed in the description of this ticket and add any that's missing? For example, I can see that 'LIH-02 Values Length Not Validated In feed_values Function' is also not included and there might be others. I'll attach the final report here so that it's unambiguous which report we are talking about.
REP-final-20230303T193754Z.pdf

[b-yap]

LI5-02 TryFrom CurrencyId implementations Contain Repeated Code
was completed by #264 (see 4th commit)

[b-yap]

#390 covers:

• IMP-01 Same Behavior Defined for different conditions
• LIE-04 Mismatch in variable name and pallet
• 4FC-02 Inconsistent comments
• PRO-01 Unhandled Error
• LIH-03 Values Length Not Validated in feed_values Function
• CLI-01 Confusion function naming
• CLI-03 Incorrect error type thrown

[b-yap]

@ebma one of the concerns of 4FC-05 Commented out code are

spacewalk/clients/vault/src/system.rs

Lines 718 to 720 in ad68c16

	// TODO
	// faucet::fund_and_register(&self.spacewalk_parachain, faucet_url, &vault_id)
	// .await?;

and

spacewalk/clients/vault/src/system.rs

Line 660 in ad68c16

// TODO fund account with faucet

Do you know what to do here, or is a separate ticket needed to address this?

[b-yap]

4FC-05 Commented out code

• some of them do not exist anymore:

• spacewalk/clients/vault/src/oracle/agent.rs

  Line 219 in 53050f5

  // self.overlay_conn.disconnect();

• spacewalk/clients/runtime/src/types.rs

  Line 77 in 53050f5

  // pub use crate::metadata::runtime_types::spacewalk_primitives::CurrencyId;

• spacewalk/primitives/src/lib.rs

  Lines 697 to 700 in 53050f5

  	#[derive(Debug)]
  	pub enum AddressConversionError {
  	// UnexpectedKeyType
  	}

[ebma]

The related ticket already exists. It's this one. I would keep the comments until we actually implemented the ticket about the faucets. If there is no other commented out code besides these, then there is nothing to do IMO.

[b-yap]

LIT-04 Reduce using unwrap() and expect() in Production Codebase
does not exist anymore:

• spacewalk/pallets/stellar-relay/src/lib.rs

  Line 619 in b5307e9

  validator_count_per_organization_map.get(organization_id).unwrap();

• spacewalk/pallets/stellar-relay/src/lib.rs

  Line 654 in b5307e9

  hasher.finalize().as_slice().try_into().unwrap()

[b-yap]

#393 covers:

• STA-01 Code duplication
• SRC-01 Unused Methods and Storage
• 4FC-05 Commented out code
• LII-01 Inconsistent match expression
• LIT-04 Reduce using unwrap() and expect() in Production Codebase
• TYP-01 Confusing Variable Naming
• LIL-01 | DEAD CODE IN execute_replace()
• LIT-03 | PANIC CAN HAPPEN BETWEEN CORRELATED STORAGE MODIFICATIONS

[b-yap]

GLOBAL-05 Unneceessary Off-Chain User Protection Mechanism

The mechanism described (reported below) in the document Spacewalk Specification (Audit) in the section security_consideration/theft_reporting adds unnecessary off-chain complexity to the bridging protocol that can be avoided without having an impact on the protocol design.
"We also don’t need theft reporting because all cases are covered except for one scenario: the user sends a payment to a vault as part of an issue request, but the vault does not call the executeIssue() extrinsic." "To protect the user in this scenario, we can implement an extra mechanism to the web app that allows the user to register new issue request

@ebma correct me if I'm wrong: the off-chain mechanism mentioned here is:

spacewalk/pallets/vault-registry/src/lib.rs

Line 780 in 5fc74e0

let call = Call::report_undercollateralized_vault { vault_id: vault };

?

[ebma]

@b-yap yes, I think it is. I'm curious however, because I think we cannot just remove that line. I think there is no other check for undercollateralized vaults. What I say in the quote about no need for theft reporting is irrelevant here, because

 let call = Call::report_undercollateralized_vault { vault_id: vault }; 

is not reporting for theft on Stellar, but it's reporting about undercollateralized vaults.

@b-yap can you check whether the current implementation checks and punishes undercollateralized vaults in another place than the line you linked to?

[ebma]

@b-yap pinging you again about the question before, before we close this ticket.

[b-yap]

@ebma
I do not see report_undercollaterized_vault() being called anywhere else besides an unsigned extrinsic call.

[ebma]

I do not see report_undercollaterized_vault() being called anywhere else besides an unsigned extrinsic call.

Can you share a link to where it's being called?

[b-yap]

https://github.com/pendulum-chain/spacewalk/blob/5fc74e08ebdcf334529ee9fb73fd2c29bdc10f7a/pallets/vault-registry/src/lib.rs#L128C6-L151
i meant when validating an extrinsic call... which is connected to the offchain worker. :/

[ebma]

I still think GLOBAL-05 is reported wrongly and they misunderstood some part of the logic. Referring to what they say in their comment, I think they misunderstand theft reporting as reporting undercollateralized vaults. Theft reporting was about reporting vaults that transfer the assets they hold somewhere else on Stellar. And we don't care about that. But undercollateralized vaults are still important for us. Thus, GLOBAL-05 is nothing we need to change. And since it's the last remaining issue of this ticket, we can close it.