Merge pull request #404 from smartcontractkit/audit-fixes

Audit fixes

contracts/src/emergency/sequencer_uptime_feed.cairo

@@ -56,7 +56,7 @@ mod SequencerUptimeFeed {
         #[substorage(v0)]
         access_control: AccessControlComponent::Storage,
         // l1 sender is an starknet validator ethereum address
-        _l1_sender: felt252,
+        _l1_sender: EthAddress,
         // maps round id to round transmission
         _round_transmissions: LegacyMap<u128, Transmission>,
         _latest_round_id: u128,
@@ -79,34 +79,43 @@ mod SequencerUptimeFeed {
     #[derive(Drop, starknet::Event)]
     struct RoundUpdated {
         status: u128,
+        #[key]
         updated_at: u64
     }
 
     #[derive(Drop, starknet::Event)]
     struct NewRound {
+        #[key]
         round_id: u128,
-        started_by: ContractAddress,
+        #[key]
+        started_by: EthAddress,
         started_at: u64
     }
 
     #[derive(Drop, starknet::Event)]
     struct AnswerUpdated {
         current: u128,
+        #[key]
         round_id: u128,
+        #[key]
         timestamp: u64
     }
 
     #[derive(Drop, starknet::Event)]
     struct UpdateIgnored {
         latest_status: u128,
+        #[key]
         latest_timestamp: u64,
         incoming_status: u128,
+        #[key]
         incoming_timestamp: u64
     }
 
     #[derive(Drop, starknet::Event)]
     struct L1SenderTransferred {
+        #[key]
         from_address: EthAddress,
+        #[key]
         to_address: EthAddress
     }
 
@@ -134,7 +143,7 @@ mod SequencerUptimeFeed {
 
         fn round_data(self: @ContractState, round_id: u128) -> Round {
             self._require_read_access();
-            assert(round_id < self._latest_round_id.read(), 'invalid round id');
+            assert(round_id <= self._latest_round_id.read(), 'invalid round id');
             let round_transmission = self._round_transmissions.read(round_id);
             Round {
                 round_id: round_id.into(),
@@ -161,6 +170,8 @@ mod SequencerUptimeFeed {
 
     #[l1_handler]
     fn update_status(ref self: ContractState, from_address: felt252, status: u128, timestamp: u64) {
+        //  Cairo enforces from_address to be a felt252 on the method signature, but we can cast it right after
+        let from_address: EthAddress = from_address.try_into().unwrap();
         assert(self._l1_sender.read() == from_address, 'EXPECTED_FROM_BRIDGE_ONLY');
 
         let latest_round_id = self._latest_round_id.read();
@@ -186,7 +197,7 @@ mod SequencerUptimeFeed {
         } else {
             // only increment round when status flips
             let round_id = latest_round_id + 1_u128;
-            self._record_round(round_id, status, timestamp);
+            self._record_round(from_address, round_id, status, timestamp);
         }
     }
 
@@ -199,21 +210,19 @@ mod SequencerUptimeFeed {
 
             let old_address = self._l1_sender.read();
 
-            if old_address != address.into() {
-                self._l1_sender.write(address.into());
+            if old_address != address {
+                self._l1_sender.write(address);
                 self
                     .emit(
                         Event::L1SenderTransferred(
-                            L1SenderTransferred {
-                                from_address: old_address.try_into().unwrap(), to_address: address
-                            }
+                            L1SenderTransferred { from_address: old_address, to_address: address }
                         )
                     );
             }
         }
 
         fn l1_sender(self: @ContractState) -> EthAddress {
-            self._l1_sender.read().try_into().unwrap()
+            self._l1_sender.read()
         }
     }
 
@@ -245,10 +254,19 @@ mod SequencerUptimeFeed {
             self.access_control.initializer();
             let round_id = 1_u128;
             let timestamp = starknet::info::get_block_timestamp();
-            self._record_round(round_id, initial_status, timestamp);
+            let from_address = EthAddress {
+                address: 0
+            }; // initial round is set by the constructor, not by an L1 sender
+            self._record_round(from_address, round_id, initial_status, timestamp);
         }
 
-        fn _record_round(ref self: ContractState, round_id: u128, status: u128, timestamp: u64) {
+        fn _record_round(
+            ref self: ContractState,
+            sender: EthAddress,
+            round_id: u128,
+            status: u128,
+            timestamp: u64
+        ) {
             self._latest_round_id.write(round_id);
             let block_info = starknet::info::get_block_info().unbox();
             let block_number = block_info.block_number;
@@ -262,8 +280,6 @@ mod SequencerUptimeFeed {
             };
             self._round_transmissions.write(round_id, round);
 
-            let sender = starknet::info::get_caller_address();
-
             self
                 .emit(
                     Event::NewRound(

contracts/src/libraries/access_control.cairo

@@ -37,11 +37,13 @@ mod AccessControlComponent {
 
     #[derive(Drop, starknet::Event)]
     struct AddedAccess {
+        #[key]
         user: ContractAddress
     }
 
     #[derive(Drop, starknet::Event)]
     struct RemovedAccess {
+        #[key]
         user: ContractAddress
     }
 

contracts/src/libraries/token/erc677.cairo

@@ -42,7 +42,9 @@ mod ERC677Component {
 
     #[derive(Drop, starknet::Event)]
     struct TransferAndCall {
+        #[key]
         from: ContractAddress,
+        #[key]
         to: ContractAddress,
         value: u256,
         data: Array<felt252>

contracts/src/libraries/upgradeable.cairo

@@ -10,6 +10,7 @@ trait IUpgradeable<TContractState> {
 
 #[derive(Drop, starknet::Event)]
 struct Upgraded {
+    #[key]
     new_impl: ClassHash
 }
 

contracts/src/multisig.cairo

@@ -107,36 +107,47 @@ mod Multisig {
 
     #[derive(Drop, starknet::Event)]
     struct TransactionSubmitted {
+        #[key]
         signer: ContractAddress,
+        #[key]
         nonce: u128,
+        #[key]
         to: ContractAddress
     }
 
     #[derive(Drop, starknet::Event)]
     struct TransactionConfirmed {
+        #[key]
         signer: ContractAddress,
+        #[key]
         nonce: u128
     }
 
     #[derive(Drop, starknet::Event)]
     struct ConfirmationRevoked {
+        #[key]
         signer: ContractAddress,
+        #[key]
         nonce: u128
     }
 
     #[derive(Drop, starknet::Event)]
     struct TransactionExecuted {
+        #[key]
         executor: ContractAddress,
+        #[key]
         nonce: u128
     }
 
     #[derive(Drop, starknet::Event)]
     struct SignersSet {
+        #[key]
         signers: Array<ContractAddress>
     }
 
     #[derive(Drop, starknet::Event)]
     struct ThresholdSet {
+        #[key]
         threshold: usize
     }
 

contracts/src/ocr2/aggregator.cairo

@@ -244,8 +244,10 @@ mod Aggregator {
 
     #[derive(Drop, starknet::Event)]
     struct NewTransmission {
+        #[key]
         round_id: u128,
         answer: u128,
+        #[key]
         transmitter: ContractAddress,
         observation_timestamp: u64,
         observers: felt252,
@@ -418,7 +420,9 @@ mod Aggregator {
 
     #[derive(Drop, starknet::Event)]
     struct ConfigSet {
+        #[key]
         previous_config_block_number: u64,
+        #[key]
         latest_config_digest: felt252,
         config_count: u64,
         oracles: Array<OracleConfig>,
@@ -848,7 +852,9 @@ mod Aggregator {
 
     #[derive(Drop, starknet::Event)]
     struct BillingAccessControllerSet {
+        #[key]
         old_controller: ContractAddress,
+        #[key]
         new_controller: ContractAddress,
     }
 
@@ -859,6 +865,7 @@ mod Aggregator {
 
     #[derive(Drop, starknet::Event)]
     struct OraclePaid {
+        #[key]
         transmitter: ContractAddress,
         payee: ContractAddress,
         amount: u256,
@@ -867,7 +874,9 @@ mod Aggregator {
 
     #[derive(Drop, starknet::Event)]
     struct LinkTokenSet {
+        #[key]
         old_link_token: ContractAddress,
+        #[key]
         new_link_token: ContractAddress
     }
 
@@ -1150,15 +1159,21 @@ mod Aggregator {
 
     #[derive(Drop, starknet::Event)]
     struct PayeeshipTransferRequested {
+        #[key]
         transmitter: ContractAddress,
+        #[key]
         current: ContractAddress,
+        #[key]
         proposed: ContractAddress,
     }
 
     #[derive(Drop, starknet::Event)]
     struct PayeeshipTransferred {
+        #[key]
         transmitter: ContractAddress,
+        #[key]
         previous: ContractAddress,
+        #[key]
         current: ContractAddress,
     }
 

packages-ts/starknet-gauntlet-multisig/src/wrapper/index.ts

@@ -285,7 +285,8 @@ export const wrapCommand = <UI, CI>(
         const txInfo = (await this.provider.provider.getTransactionReceipt(
           tx.hash,
         )) as InvokeTransactionReceiptResponse
-        proposalId = Number(num.hexToDecimalString((txInfo.events[0] as any).data[1]))
+        // TODO: use contract.parseEvents?
+        proposalId = Number(num.hexToDecimalString((txInfo.events[0] as any).keys[2])) // 0 == event_id, 1 == executor, 2 == nonce/proposal_id
       }
 
       const data = await this.afterExecute(result, proposalId)

packages-ts/starknet-gauntlet-ocr2/src/lib/encoding.ts

@@ -4,16 +4,16 @@ import { feltsToBytes } from '@chainlink/starknet-gauntlet'
 export const decodeOffchainConfigFromEventData = (data: string[]): encoding.OffchainConfig => {
   // The ConfigSet event is defined as:
   // fn ConfigSet(
-  //   previous_config_block_number: u64,
-  //   latest_config_digest: felt252,
+  //   previous_config_block_number: u64, (key)
+  //   latest_config_digest: felt252, (key)
   //   config_count: u64,
   //   oracles: Array<OracleConfig>,
   //   f: u8,
   //   onchain_config: Array<felt252>,
   //   offchain_config_version: u64,
   //   offchain_config: Array<felt252>,
   //)
-  const oraclesLenIndex = 3
+  const oraclesLenIndex = 1
   const oraclesLen = Number(BigInt(data[oraclesLenIndex]))
   const oracleStructSize = 2
   const fIndex = oraclesLenIndex + oraclesLen * oracleStructSize + 1

packages-ts/starknet-gauntlet-ocr2/test/commands/ocr2.test.ts

@@ -180,7 +180,7 @@ describe('OCR2 Contract', () => {
       // reconstruct signers array from event
       const eventSigners: bigint[] = []
       for (let i = 0; i < signers.length; i++) {
-        const signer = BigInt(eventData[4 + 2 * i]) // split according to event structure
+        const signer = BigInt(eventData[2 + 2 * i]) // split according to event structure
         eventSigners.push(signer)
       }