Prevent coin transfer to SC addresses

massa-execution-worker/src/context.rs

@@ -455,14 +455,6 @@ impl ExecutionContext {
 
     /// Creates a new smart contract address with initial bytecode, and returns this address
     pub fn create_new_sc_address(&mut self, bytecode: Bytecode) -> Result<Address, ExecutionError> {
-        // TODO: collision problem:
-        //  prefix addresses to know if they are SCs or normal,
-        //  otherwise people can already create new accounts by sending coins to the right hash
-        //  they won't have ownership over it but this can still be unexpected
-        //  to have initial extra coins when an address is created
-        //  It may also induce that for read-only calls.
-        //  https://github.com/massalabs/massa/issues/2331
-
         // deterministically generate a new unique smart contract address
         let slot_timestamp = get_block_slot_timestamp(
             self.config.thread_count,
@@ -475,20 +467,48 @@ impl ExecutionContext {
         // create a seed from the current slot
         let mut data: Vec<u8> = self.slot.to_bytes_key().to_vec();
         // add the index of the created address within this context to the seed
-        data.append(&mut self.created_addr_index.to_be_bytes().to_vec());
+        data.extend(self.created_addr_index.to_be_bytes());
         // add a flag on whether we are in read-only mode or not to the seed
         // this prevents read-only contexts from shadowing existing addresses
         if self.read_only {
             data.push(0u8);
         } else {
             data.push(1u8);
         }
-        // hash the seed to get a unique address
-        let hash = Hash::compute_from(&data);
-        let address = self.address_factory.create(
-            &AddressArgs::SC { hash },
-            FactoryStrategy::At(slot_timestamp),
-        )?;
+        // hash the seed to get an address
+        let mut hash = Hash::compute_from(&data);
+        let mut nonce = 0u64;
+        let mut address;
+
+        // Loop over nonces until we find an address that doesn't exist in the speculative ledger.
+        // Note that this loop is here for robustness, and should not be looping because
+        // even through the SC addresses are predictable, nobody can create them beforehand because:
+        // - their category is "SC" and not "USER" so they can't be derived from a public key
+        // - sending tokens to the target SC address to create it by funding is not allowed because transactions towards SC addresses are not allowed
+        loop {
+            address = self.address_factory.create(
+                &AddressArgs::SC { hash },
+                FactoryStrategy::At(slot_timestamp),
+            )?;
+            // check if this address already exists in the speculative ledger
+            if !self.speculative_ledger.entry_exists(&address) {
+                // if not, we can use it
+                break;
+            }
+            // otherwise, increment the nonce to get a new hash and try again
+            nonce = nonce.checked_add(1).ok_or_else(|| {
+                ExecutionError::RuntimeError("nonce overflow when creating SC address".into())
+            })?;
+            let mut hash_content = hash.to_bytes().to_vec();
+            hash_content.extend(nonce.to_be_bytes());
+            hash = Hash::compute_from(&hash_content);
+        }
+        if nonce > 0 {
+            warn!(
+                "smart contract address generation required {} nonces",
+                nonce
+            );
+        }
 
         // add this address with its bytecode to the speculative ledger
         self.speculative_ledger.create_new_sc_address(
@@ -664,13 +684,25 @@ impl ExecutionContext {
     ) -> Result<(), ExecutionError> {
         if let Some(from_addr) = &from_addr {
             // check access rights
-            if check_rights && !self.has_write_rights_on(from_addr) {
-                return Err(ExecutionError::RuntimeError(format!(
-                    "spending from address {} is not allowed in this context",
-                    from_addr
-                )));
-            }
+            if check_rights {
+                // ensure we can't spend from an address on which we have no write access
+                if !self.has_write_rights_on(from_addr) {
+                    return Err(ExecutionError::RuntimeError(format!(
+                        "spending from address {} is not allowed in this context",
+                        from_addr
+                    )));
+                }
 
+                // ensure we can't transfer towards SC addresses on which we have no write access
+                if let Some(to_addr) = &to_addr {
+                    if matches!(to_addr, Address::SC(..)) && !self.has_write_rights_on(to_addr) {
+                        return Err(ExecutionError::RuntimeError(format!(
+                            "crediting SC address {} is not allowed without write access to it",
+                            to_addr
+                        )));
+                    }
+                }
+            }
             // control vesting min_balance for sender address
             self.vesting_manager
                 .check_vesting_transfer_coins(self, from_addr, amount)?;

massa-execution-worker/src/execution.rs

@@ -645,7 +645,7 @@ impl ExecutionState {
             )));
         }
 
-        // add rolls to the buyer withing the context
+        // add rolls to the buyer within the context
         context.add_rolls(&buyer_addr, *roll_count);
 
         Ok(())
@@ -686,7 +686,7 @@ impl ExecutionState {
 
         // send `roll_price` * `roll_count` coins from the sender to the recipient
         if let Err(err) =
-            context.transfer_coins(Some(sender_addr), Some(*recipient_address), *amount, false)
+            context.transfer_coins(Some(sender_addr), Some(*recipient_address), *amount, true)
         {
             return Err(ExecutionError::TransactionError(format!(
                 "transfer of {} coins from {} to {} failed: {}",
@@ -810,19 +810,21 @@ impl ExecutionState {
                 },
             ];
 
-            // Debit the sender's balance with the coins to transfer
-            if let Err(err) = context.transfer_coins(Some(sender_addr), None, coins, false) {
+            // Ensure that the target address is an SC address
+            if !matches!(target_addr, Address::SC(..)) {
                 return Err(ExecutionError::RuntimeError(format!(
-                    "failed to debit operation sender {} with {} operation coins: {}",
-                    sender_addr, coins, err
+                    "cannot callSC towards non-SC address {}",
+                    target_addr
                 )));
             }
 
-            // Credit the operation target with coins.
-            if let Err(err) = context.transfer_coins(None, Some(target_addr), coins, false) {
+            // Transfer coins from the sender to the target
+            if let Err(err) =
+                context.transfer_coins(Some(sender_addr), Some(target_addr), coins, false)
+            {
                 return Err(ExecutionError::RuntimeError(format!(
-                    "failed to credit operation target {} with {} operation coins: {}",
-                    target_addr, coins, err
+                    "failed to transfer {} operation coins from {} to {}: {}",
+                    coins, sender_addr, target_addr, err
                 )));
             }
 
@@ -896,18 +898,21 @@ impl ExecutionState {
                 },
             ];
 
-            // If there is no target bytecode or if message data is invalid,
-            // reimburse sender with coins and quit
+            // if the target address is not SC: fail
+            if !matches!(message.destination, Address::SC(..)) {
+                let err = ExecutionError::RuntimeError(
+                    "the target address is not a smart contract address".into(),
+                );
+                context.reset_to_snapshot(context_snapshot, err.clone());
+                context.cancel_async_message(&message);
+                return Err(err);
+            }
+
+            // if there is no bytecode: fail
             let bytecode = match bytecode {
-                Some(bc) => bc,
-                bc => {
-                    let err = if bc.is_none() {
-                        ExecutionError::RuntimeError("no target bytecode found".into())
-                    } else {
-                        ExecutionError::RuntimeError(
-                            "message data does not convert to utf-8".into(),
-                        )
-                    };
+                Some(bytecode) => bytecode,
+                None => {
+                    let err = ExecutionError::RuntimeError("no target bytecode found".into());
                     context.reset_to_snapshot(context_snapshot, err.clone());
                     context.cancel_async_message(&message);
                     return Err(err);

massa-execution-worker/src/interface_impl.rs

@@ -170,6 +170,11 @@ impl Interface for InterfaceImpl {
         // get target address
         let to_address = Address::from_str(address)?;
 
+        // check that the target address is an SC address
+        if !matches!(to_address, Address::SC(..)) {
+            bail!("called address {} is not an SC address", to_address);
+        }
+
         // write-lock context
         let mut context = context_guard!(self);
 
@@ -187,7 +192,9 @@ impl Interface for InterfaceImpl {
 
         // transfer coins from caller to target address
         let coins = Amount::from_raw(raw_coins);
-        if let Err(err) = context.transfer_coins(Some(from_address), Some(to_address), coins, true)
+        // note: rights are not checked here we checked that to_address is an SC address above
+        // and we know that the sender is at the top of the call stack
+        if let Err(err) = context.transfer_coins(Some(from_address), Some(to_address), coins, false)
         {
             bail!(
                 "error transferring {} coins from {} to {}: {}",
@@ -740,19 +747,26 @@ impl Interface for InterfaceImpl {
         if validity_end.1 >= self.config.thread_count {
             bail!("validity end thread exceeds the configuration thread count")
         }
+        let target_addr = Address::from_str(target_address)?;
+
+        // check that the target address is an SC address
+        if !matches!(target_addr, Address::SC(..)) {
+            bail!("target address is not a smart contract address")
+        }
+
         let mut execution_context = context_guard!(self);
         let emission_slot = execution_context.slot;
         let emission_index = execution_context.created_message_index;
         let sender = execution_context.get_current_address()?;
         let coins = Amount::from_raw(raw_coins);
-        let fee = Amount::from_raw(raw_fee);
         execution_context.transfer_coins(Some(sender), None, coins, true)?;
+        let fee = Amount::from_raw(raw_fee);
         execution_context.transfer_coins(Some(sender), None, fee, true)?;
         execution_context.push_new_message(AsyncMessage::new_with_hash(
             emission_slot,
             emission_index,
             sender,
-            Address::from_str(target_address)?,
+            target_addr,
             target_handler.to_string(),
             max_gas,
             fee,