Merge pull request #24 from multiversx/audit_v1_fixes

Audit v1 fixes

.github/workflows/actions.yml

@@ -14,18 +14,18 @@ permissions:
 jobs:
   contracts:
     name: Contracts
-    uses: multiversx/mx-sc-actions/.github/workflows/contracts.yml@v3.2.0
+    uses: multiversx/mx-sc-actions/.github/workflows/contracts.yml@v3.3.1
     with:
-      rust-toolchain: 1.78.0
-      pip-mxpy-args: "multiversx-sdk-cli==v9.5.5"
+      rust-toolchain: 1.81.0
+      pip-mxpy-args: "multiversx-sdk-cli==v9.7.0"
     secrets:
       token: ${{ secrets.GITHUB_TOKEN }}
 
   tests:
     name: xSuite Tests
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@v3
 
       - name: Install rust
         uses: actions-rust-lang/setup-rust-toolchain@v1
@@ -35,17 +35,18 @@ jobs:
 
       - name: Install prerequisites
         run: |
-          pipx install multiversx-sdk-cli==v9.5.5
+          pipx install "multiversx-sdk-cli==v9.7.0"
           
           wget -O binaryen.tar.gz https://github.com/WebAssembly/binaryen/releases/download/version_112/binaryen-version_112-x86_64-linux.tar.gz
           tar -xf binaryen.tar.gz
           cp binaryen-version_112/bin/wasm-opt $HOME/.local/bin
 
           sudo apt install -y wabt=1.0.27-1
 
+          cargo install wasm-opt
           cargo install twiggy
           
-          cargo install multiversx-sc-meta --locked
+          cargo install multiversx-sc-meta --version 0.50.3 --locked
 
           which mxpy
           which wasm-opt

gas-service/src/lib.rs

@@ -236,8 +236,6 @@ pub trait GasService: events::Events {
         let tokens_vec = tokens.into_vec();
         let amounts_vec = amounts.into_vec();
 
-        let mut payments = ManagedVec::new();
-
         for index in 0..tokens_length {
             let token: EgldOrEsdtTokenIdentifier = tokens_vec.get(index);
             let amount = amounts_vec.get(index).clone_value();
@@ -251,13 +249,9 @@ pub trait GasService: events::Events {
                     self.send().direct_egld(receiver, &amount);
                 }
             } else if amount <= balance {
-                payments.push(EsdtTokenPayment::new(token.unwrap_esdt(), 0, amount));
+                self.send().direct_esdt(receiver, &token.unwrap_esdt(), 0, &amount)
             }
         }
-
-        if !payments.is_empty() {
-            self.send().direct_multi(receiver, &payments);
-        }
     }
 
     #[endpoint(refund)]
@@ -288,7 +282,11 @@ pub trait GasService: events::Events {
 
     #[endpoint(setGasCollector)]
     fn set_gas_collector(&self, gas_collector: &ManagedAddress) {
-        self.require_only_collector();
+        let caller = self.blockchain().get_caller();
+        let collector = self.gas_collector().get();
+        let owner = self.blockchain().get_owner_address();
+
+        require!(caller == collector || caller == owner, "Not collector or owner");
 
         self.gas_collector().set(gas_collector);
     }

gateway/src/auth.rs

@@ -169,13 +169,15 @@ pub trait AuthModule: events::Events {
     ) -> ManagedByteArray<KECCAK256_RESULT_LEN> {
         let mut encoded = ManagedBuffer::new();
 
-        for signer in signers.signers.iter() {
-            encoded.append(signer.signer.as_managed_buffer());
-            encoded.append(&signer.weight.to_bytes_be_buffer());
-        }
-
-        encoded.append(&signers.threshold.to_bytes_be_buffer());
-        encoded.append(signers.nonce.as_managed_buffer());
+        signers.signers
+            .dep_encode(&mut encoded)
+            .unwrap_or_else(|_| sc_panic!("Could not encode signers"));
+        signers.threshold
+            .dep_encode(&mut encoded)
+            .unwrap_or_else(|_| sc_panic!("Could not encode threshold"));
+        signers.nonce
+            .dep_encode(&mut encoded)
+            .unwrap_or_else(|_| sc_panic!("Could not encode nonce"));
 
         self.crypto().keccak256(encoded)
     }

gateway/src/constants.rs

@@ -1,7 +1,7 @@
+use core::convert::TryFrom;
 use multiversx_sc::api::ED25519_KEY_BYTE_LEN;
 use multiversx_sc::api::ED25519_SIGNATURE_BYTE_LEN;
 use multiversx_sc::api::KECCAK256_RESULT_LEN;
-use core::convert::TryFrom;
 
 multiversx_sc::imports!();
 multiversx_sc::derive_imports!();
@@ -42,6 +42,12 @@ pub struct Proof<M: ManagedTypeApi> {
     pub signatures: ManagedVec<M, Option<ManagedByteArray<M, ED25519_SIGNATURE_BYTE_LEN>>>,
 }
 
+#[derive(TypeAbi, TopDecode, TopEncode, NestedEncode)]
+pub struct CrossChainId<M: ManagedTypeApi> {
+    pub source_chain: ManagedBuffer<M>,
+    pub message_id: ManagedBuffer<M>,
+}
+
 const MESSAGE_EXECUTED: &[u8; 1] = b"1";
 
 #[derive(TypeAbi, PartialEq, Default)]
@@ -53,32 +59,28 @@ pub enum MessageState<M: ManagedTypeApi> {
 }
 
 impl<M: ManagedTypeApi> codec::TopEncode for MessageState<M> {
-    fn top_encode_or_handle_err<O, H>(
-        &self,
-        output: O,
-        h: H,
-    ) -> Result<(), H::HandledErr>
-        where
-            O: codec::TopEncodeOutput,
-            H: codec::EncodeErrorHandler,
+    fn top_encode_or_handle_err<O, H>(&self, output: O, h: H) -> Result<(), H::HandledErr>
+    where
+        O: codec::TopEncodeOutput,
+        H: codec::EncodeErrorHandler,
     {
         match self {
             MessageState::NonExistent => codec::TopEncode::top_encode_or_handle_err(&"", output, h),
             MessageState::Approved(hash) => {
                 codec::TopEncode::top_encode_or_handle_err(hash.as_managed_buffer(), output, h)
-            },
+            }
             MessageState::Executed => {
                 codec::TopEncode::top_encode_or_handle_err(MESSAGE_EXECUTED, output, h)
-            },
+            }
         }
     }
 }
 
 impl<M: ManagedTypeApi> codec::TopDecode for MessageState<M> {
     fn top_decode_or_handle_err<I, H>(input: I, h: H) -> Result<Self, H::HandledErr>
-        where
-            I: codec::TopDecodeInput,
-            H: codec::DecodeErrorHandler,
+    where
+        I: codec::TopDecodeInput,
+        H: codec::DecodeErrorHandler,
     {
         let decoded_input = ManagedBuffer::top_decode_or_handle_err(input, h)?;
         if decoded_input.is_empty() {

gateway/src/events.rs

@@ -19,7 +19,6 @@ pub trait Events {
     #[event("message_approved_event")]
     fn message_approved_event(
         &self,
-        #[indexed] command_id: &ManagedByteArray<KECCAK256_RESULT_LEN>,
         #[indexed] source_chain: ManagedBuffer,
         #[indexed] message_id: ManagedBuffer,
         #[indexed] source_address: ManagedBuffer,
@@ -30,7 +29,6 @@ pub trait Events {
     #[event("message_executed_event")]
     fn message_executed_event(
         &self,
-        #[indexed] command_id: &ManagedByteArray<KECCAK256_RESULT_LEN>,
         #[indexed] source_chain: &ManagedBuffer,
         #[indexed] message_id: &ManagedBuffer,
     );

gateway/src/lib.rs

@@ -74,15 +74,15 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events
             WeightedSigners::<Self::Api>::top_decode(new_signers)
                 .unwrap_or_else(|_| sc_panic!("Could not decode new signers"));
 
-        let enfore_rotation_delay = self.blockchain().get_caller() != self.operator().get();
+        let enforce_rotation_delay = self.blockchain().get_caller() != self.operator().get();
         let is_latest_signers = self.validate_proof(data_hash, proof);
 
         require!(
-            !enfore_rotation_delay || is_latest_signers,
+            !enforce_rotation_delay || is_latest_signers,
             "Not latest signers"
         );
 
-        self.rotate_signers_raw(new_signers, enfore_rotation_delay);
+        self.rotate_signers_raw(new_signers, enforce_rotation_delay);
     }
 
     /// Public Methods
@@ -107,28 +107,30 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events
     #[endpoint(validateMessage)]
     fn validate_message(
         &self,
-        source_chain: &ManagedBuffer,
-        message_id: &ManagedBuffer,
+        source_chain: ManagedBuffer,
+        message_id: ManagedBuffer,
         source_address: &ManagedBuffer,
         payload_hash: &ManagedByteArray<KECCAK256_RESULT_LEN>,
     ) -> bool {
-        let command_id = self.message_to_command_id(source_chain, message_id);
-
-        let message_hash = self.message_hash(
+        let cross_chain_id = CrossChainId {
             source_chain,
             message_id,
+        };
+
+        let message_hash = self.message_hash(
+            &cross_chain_id,
             source_address,
             &self.blockchain().get_caller(),
             payload_hash,
         );
 
-        let messages_mapper = self.messages(&command_id);
+        let messages_mapper = self.messages(&cross_chain_id);
         let valid = messages_mapper.get() == MessageState::Approved(message_hash);
 
         if valid {
             messages_mapper.set(MessageState::Executed);
 
-            self.message_executed_event(&command_id, source_chain, message_id);
+            self.message_executed_event(&cross_chain_id.source_chain, &cross_chain_id.message_id);
         }
 
         valid
@@ -137,17 +139,19 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events
     // Self Functions
 
     fn approve_message(&self, message: Message<Self::Api>) {
-        let command_id = self.message_to_command_id(&message.source_chain, &message.message_id);
+        let cross_chain_id = CrossChainId {
+            source_chain: message.source_chain,
+            message_id: message.message_id,
+        };
 
-        let messages_mapper = self.messages(&command_id);
+        let messages_mapper = self.messages(&cross_chain_id);
 
         if messages_mapper.get() != MessageState::NonExistent {
             return;
         }
 
         let message_hash = self.message_hash(
-            &message.source_chain,
-            &message.message_id,
+            &cross_chain_id,
             &message.source_address,
             &message.contract_address,
             &message.payload_hash,
@@ -156,9 +160,8 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events
         messages_mapper.set(MessageState::Approved(message_hash));
 
         self.message_approved_event(
-            &command_id,
-            message.source_chain,
-            message.message_id,
+            cross_chain_id.source_chain,
+            cross_chain_id.message_id,
             message.source_address,
             message.contract_address,
             message.payload_hash,
@@ -167,19 +170,25 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events
 
     fn message_hash(
         &self,
-        source_chain: &ManagedBuffer,
-        message_id: &ManagedBuffer,
+        cross_chain_id: &CrossChainId<Self::Api>,
         source_address: &ManagedBuffer,
         contract_address: &ManagedAddress,
         payload_hash: &ManagedByteArray<KECCAK256_RESULT_LEN>,
     ) -> ManagedByteArray<KECCAK256_RESULT_LEN> {
         let mut encoded = ManagedBuffer::new();
 
-        encoded.append(source_chain);
-        encoded.append(message_id);
-        encoded.append(source_address);
-        encoded.append(contract_address.as_managed_buffer());
-        encoded.append(payload_hash.as_managed_buffer());
+        cross_chain_id
+            .dep_encode(&mut encoded)
+            .unwrap_or_else(|_| sc_panic!("Could not encode cross chain id"));
+        source_address
+            .dep_encode(&mut encoded)
+            .unwrap_or_else(|_| sc_panic!("Could not encode source address"));
+        contract_address
+            .dep_encode(&mut encoded)
+            .unwrap_or_else(|_| sc_panic!("Could not encode contract address"));
+        payload_hash
+            .dep_encode(&mut encoded)
+            .unwrap_or_else(|_| sc_panic!("Could not encode payload hash"));
 
         self.crypto().keccak256(encoded)
     }
@@ -199,58 +208,44 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events
         self.crypto().keccak256(encoded)
     }
 
-    fn message_to_command_id(
-        &self,
-        source_chain: &ManagedBuffer,
-        message_id: &ManagedBuffer,
-    ) -> ManagedByteArray<KECCAK256_RESULT_LEN> {
-        // Axelar doesn't allow `sourceChain` to contain '_', hence this encoding is umambiguous
-        let mut encoded = ManagedBuffer::new();
-
-        encoded.append(source_chain);
-        encoded.append(&ManagedBuffer::from("_"));
-        encoded.append(message_id);
-
-        self.crypto().keccak256(encoded)
-    }
-
     #[view(isMessageApproved)]
     fn is_message_approved(
         &self,
-        source_chain: &ManagedBuffer,
-        message_id: &ManagedBuffer,
+        source_chain: ManagedBuffer,
+        message_id: ManagedBuffer,
         source_address: &ManagedBuffer,
         contract_address: &ManagedAddress,
         payload_hash: &ManagedByteArray<KECCAK256_RESULT_LEN>,
     ) -> bool {
-        let command_id = self.message_to_command_id(source_chain, message_id);
-
-        let message_hash = self.message_hash(
+        let cross_chain_id = CrossChainId {
             source_chain,
             message_id,
+        };
+
+        let message_hash = self.message_hash(
+            &cross_chain_id,
             source_address,
             contract_address,
             payload_hash,
         );
 
-        self.messages(&command_id).get() == MessageState::Approved(message_hash)
+        self.messages(&cross_chain_id).get() == MessageState::Approved(message_hash)
     }
 
     #[view(isMessageExecuted)]
-    fn is_message_executed(
-        &self,
-        source_chain: &ManagedBuffer,
-        message_id: &ManagedBuffer,
-    ) -> bool {
-        self.messages(&self.message_to_command_id(source_chain, message_id))
-            .get()
-            == MessageState::Executed
+    fn is_message_executed(&self, source_chain: ManagedBuffer, message_id: ManagedBuffer) -> bool {
+        let cross_chain_id = CrossChainId {
+            source_chain,
+            message_id,
+        };
+
+        self.messages(&cross_chain_id).get() == MessageState::Executed
     }
 
     #[view(messages)]
     #[storage_mapper("messages")]
     fn messages(
         &self,
-        command_id: &ManagedByteArray<KECCAK256_RESULT_LEN>,
+        cross_chain_id: &CrossChainId<Self::Api>,
     ) -> SingleValueMapper<MessageState<Self::Api>>;
 }