Fixes fillNodeRoleSlots() to not remove from array while it is being accessed

contracts/FlowIDTableStaking.cdc

@@ -1156,7 +1156,7 @@ access(all) contract FlowIDTableStaking {
             let slotLimits: {UInt8: UInt16} = FlowIDTableStaking.getRoleSlotLimits()
 
             let openSlots = FlowIDTableStaking.getOpenNodeSlots()
-
+            
             let nodesToAdd: [String] = []
 
             // Load and reset the candidate node list
@@ -1167,6 +1167,7 @@ access(all) contract FlowIDTableStaking {
             for role in currentNodeCount.keys {
 
                 let candidateNodesForRole = candidateNodes[role]!
+                let nodesToRemoveFromCandidateNodes: [String] = []
 
                 if currentNodeCount[role]! >= slotLimits[role]! {
                     // if all slots are full, remove and refund all pending nodes
@@ -1199,7 +1200,7 @@ access(all) contract FlowIDTableStaking {
                     for nodeIndex in deletionList.keys {
                         let nodeID = candidateNodesForRole.keys[nodeIndex]
                         self.removeAndRefundNodeRecord(nodeID)
-                        candidateNodesForRole.remove(key: nodeID)
+                        nodesToRemoveFromCandidateNodes.append(nodeID)
                     }
 
                     // Set the current node count for the role to the limit for the role, since they were all filled
@@ -1212,6 +1213,11 @@ access(all) contract FlowIDTableStaking {
                     currentNodeCount[role] = currentNodeCount[role]! + UInt16(candidateNodesForRole.keys.length)
                 }
 
+                // Remove the refunded nodes from the candidate nodes list
+                for node in nodesToRemoveFromCandidateNodes {
+                    candidateNodesForRole.remove(key: node)
+                }
+
                 nodesToAdd.appendAll(candidateNodesForRole.keys)
 
                 // Add the desired open slots for each role to the current node count

lib/go/templates/idtable_staking_templates.go

@@ -576,3 +576,17 @@ func GenerateGetApprovedNodesScript(env Environment) []byte {
 
 	return []byte(ReplaceAddresses(code, env))
 }
+
+func GenerateEndStakingTestScript(env Environment) []byte {
+	code := `
+		import FlowIDTableStaking from "FlowIDTableStaking"
+
+		access(all) fun main() {
+			let acct = getAuthAccount<auth(BorrowValue) &Account>("FlowIDTableStaking")
+			let adminRef = acct.storage.borrow<&FlowIDTableStaking.Admin>(from: FlowIDTableStaking.StakingAdminStoragePath)
+            	?? panic("Could not borrow reference to staking admin")
+
+        	adminRef.endStakingAuction()
+		}`
+	return []byte(ReplaceAddresses(code, env))
+}

lib/go/test/flow_idtable_nodes_test.go

@@ -204,7 +204,6 @@ func TestIDTableManyNodes(t *testing.T) {
 	})
 
 	approvedNodesDict := generateCadenceNodeDictionary(approvedNodesStringArray)
-
 	// End staking auction
 	t.Run("Should end staking auction, pay rewards, and move tokens", func(t *testing.T) {
 
@@ -362,6 +361,139 @@ func TestIDTableManyNodes(t *testing.T) {
 
 }
 
+func TestIDTableOutOfBoundsAccess(t *testing.T) {
+
+	t.Parallel()
+
+	b, adapter := newBlockchain(emulator.WithTransactionMaxGasLimit(10000000))
+
+	env := templates.Environment{
+		FungibleTokenAddress: emulatorFTAddress,
+		FlowTokenAddress:     emulatorFlowTokenAddress,
+		BurnerAddress:        emulatorServiceAccount,
+		StorageFeesAddress:   emulatorServiceAccount,
+	}
+
+	accountKeys := test.AccountKeyGenerator()
+
+	// Create new keys for the ID table account
+	IDTableAccountKey, IDTableSigner := accountKeys.NewWithSigner()
+	idTableAddress, _ := deployStakingContract(t, b, IDTableAccountKey, IDTableSigner, &env, true, []uint64{10000, 10000, 10000, 10000, 10000})
+
+	env.IDTableAddress = idTableAddress.Hex()
+
+	var nodeAccountKey *flow.AccountKey
+	var nodeSigner crypto.Signer
+	var nodeAddress flow.Address
+
+	// Create a new node account for nodes
+	nodeAccountKey, nodeSigner = accountKeys.NewWithSigner()
+	nodeAddress, _ = adapter.CreateAccount(context.Background(), []*flow.AccountKey{nodeAccountKey}, nil)
+
+	approvedNodes := make([]cadence.Value, numberOfNodes)
+	approvedNodesStringArray := make([]string, numberOfNodes)
+	nodeRoles := make([]cadence.Value, numberOfNodes)
+	nodeNetworkingAddresses := make([]cadence.Value, numberOfNodes)
+	nodeNetworkingKeys := make([]cadence.Value, numberOfNodes)
+	nodeStakingKeys := make([]cadence.Value, numberOfNodes)
+	nodeStakingAmounts := make([]cadence.Value, numberOfNodes)
+	nodePaths := make([]cadence.Value, numberOfNodes)
+
+	totalMint := numberOfNodes * nodeMintAmount
+	mintAmount := fmt.Sprintf("%d.0", totalMint)
+
+	script := templates.GenerateMintFlowScript(env)
+	tx := createTxWithTemplateAndAuthorizer(b, script, b.ServiceKey().Address)
+	_ = tx.AddArgument(cadence.NewAddress(nodeAddress))
+	_ = tx.AddArgument(CadenceUFix64(mintAmount))
+
+	signAndSubmit(
+		t, b, tx,
+		[]flow.Address{},
+		[]crypto.Signer{},
+		false,
+	)
+
+	tx = flow.NewTransaction().
+		SetScript(templates.GenerateStartStakingScript(env)).
+		SetGasLimit(9999).
+		SetProposalKey(b.ServiceKey().Address, b.ServiceKey().Index, b.ServiceKey().SequenceNumber).
+		SetPayer(b.ServiceKey().Address).
+		AddAuthorizer(idTableAddress)
+
+	signAndSubmit(
+		t, b, tx,
+		[]flow.Address{idTableAddress},
+		[]crypto.Signer{IDTableSigner},
+		false,
+	)
+
+	t.Run("Should be able to create many valid Node structs", func(t *testing.T) {
+
+		for i := 0; i < numberOfNodes; i++ {
+
+			id := fmt.Sprintf("%064d", i)
+
+			approvedNodes[i] = CadenceString(id)
+			approvedNodesStringArray[i] = id
+
+			nodeRoles[i] = cadence.NewUInt8(uint8((i % 4) + 1))
+
+			networkingAddress := fmt.Sprintf("%0128d", i)
+
+			nodeNetworkingAddresses[i] = CadenceString(networkingAddress)
+
+			_, stakingKey, _, networkingKey := generateKeysForNodeRegistration(t)
+
+			nodeNetworkingKeys[i] = CadenceString(networkingKey)
+
+			nodeStakingKeys[i] = CadenceString(stakingKey)
+
+			tokenAmount, err := cadence.NewUFix64("1500000.0")
+			require.NoError(t, err)
+
+			nodeStakingAmounts[i] = tokenAmount
+			nodePaths[i] = cadence.Path{Domain: common.PathDomainStorage, Identifier: fmt.Sprintf("node%06d", i)}
+
+		}
+
+		assertCandidateLimitsEquals(t, b, env, []uint64{10000, 10000, 10000, 10000, 10000})
+
+		tx := flow.NewTransaction().
+			SetScript(templates.GenerateRegisterManyNodesScript(env)).
+			SetGasLimit(5000000).
+			SetProposalKey(b.ServiceKey().Address, b.ServiceKey().Index, b.ServiceKey().SequenceNumber).
+			SetPayer(b.ServiceKey().Address).
+			AddAuthorizer(nodeAddress)
+
+		tx.AddArgument(cadence.NewArray(approvedNodes))
+		tx.AddArgument(cadence.NewArray(nodeRoles))
+		tx.AddArgument(cadence.NewArray(nodeNetworkingAddresses))
+		tx.AddArgument(cadence.NewArray(nodeNetworkingKeys))
+		tx.AddArgument(cadence.NewArray(nodeStakingKeys))
+		tx.AddArgument(cadence.NewArray(nodeStakingAmounts))
+		tx.AddArgument(cadence.NewArray(nodePaths))
+
+		signAndSubmit(
+			t, b, tx,
+			[]flow.Address{nodeAddress},
+			[]crypto.Signer{nodeSigner},
+			false,
+		)
+	})
+
+	t.Run("Should end staking auction with no approved nodes which should not fail because of out of bounds array access", func(t *testing.T) {
+
+		setNodeRoleSlotLimits(t, b, env, idTableAddress, IDTableSigner, [5]uint16{5, 5, 5, 5, 2})
+
+		scriptResult, err := b.ExecuteScript(templates.GenerateEndStakingTestScript(env), nil)
+		require.NoError(t, err)
+		if !assert.True(t, scriptResult.Succeeded()) {
+			t.Log(scriptResult.Error.Error())
+		}
+	})
+}
+
 func TestIDTableUnstakeAllManyDelegators(t *testing.T) {
 
 	t.Parallel()