Rangers Protocol

Commit: - [`0248879b845ff1a9e790580655cf7daf784536d1`](https://github.com/rangersprotocolcode/go-rangers/tree/0248879b845ff1a9e790580655cf7daf784536d1) Files: - src/common/sign.go - src/core/sync_msg.go - src/core/sync_processor.go The `SignInfo.Id` property is generally used as peer identifier in order to detect bad nodes and to send back responses after answers. However, the codebase lacks proper checks that `SignInfo.Id` represents a legitimate peer identity. Moreover, the `SignData.ValidateSign` function, fetches the public key of the signed payload from the message itself, instead of matching it with a whitelist or blacklist of already known peers. This means that any peer could include an arbitrary `Id` and impersonate any other peer. A malicious node could leverage such situation and, based on the involved functionality, bring the target node in an inconsistent state or circumvent any identity based protection mechanism. The issue involves the following functionalities. - The `chainInfoNotifyHandler` adds to the internal `candidatePool` map the potential updates of the chain. When doing such operation, the `chainInfo.SignInfo.Id` property provides information on the sender peer. If such peer was flagged as "evil" in the `PeerManager` then such message is not considered. Since any peer could present an arbitrary `Id`, a malicious node could leverage such situation and bring a target node in an inconsistent state, letting it accept as global state some chain incorrect chain information. - The `blockChainPieceReqHandler` method is supposed to handle the incoming request of a blockchain piece. Since the `Id` can be arbitrarily set by the sender, an attacker could exploit the whole network to flood with responses a target node which, instead, did not perform any request. Such vulnerability enables and attacker to use the whole network as amplification system for a DDoS attack. - The `blockChainPieceHandler` handle manages incoming chain information filtering out messages coming from peers other than the expected candidate. Any malicious node can arbitrarily set the `chainPieceInfo.SignInfo.Id` property and circumvent such check on the sender. - The `blockResponseMsgHandler` and `groupResponseMsgHandler` handlers respectively check the received block or group response comes from the expected candidate peer by checking its Id. Any malicious node can impersonate the expected candidate peer and provide wrong information on the synchronized block.

Commit: - [`0248879b845ff1a9e790580655cf7daf784536d1`](https://github.com/rangersprotocolcode/go-rangers/tree/0248879b845ff1a9e790580655cf7daf784536d1) Files - src/core/sync_msg.go The function `verifyBlockChainPiece()` does not verify the `topHeader` is legitimate via `consensusHelper.VerifyBlockHeader`. Per the comment of `VerifyBlockHeader`. ```go //verify the blockheader: mainly verify the group signature VerifyBlockHeader(bh *BlockHeader) (bool, error) ``` A malicious node can send a chain piece of info with a malformed group signature to parse the verification process of `verifyBlockChainPiece()`, which may bring unexpected errors.

Commit: - [`0248879b845ff1a9e790580655cf7daf784536d1`](https://github.com/rangersprotocolcode/go-rangers/tree/0248879b845ff1a9e790580655cf7daf784536d1) Files: - src/storage/trie/database.go The function `Commit()` iterates over all the children of a particular node, and writes them out to disk. Before writing nodes to disk, The function `Commit()` places a read-only locker by `db.lock.RLock()`, However, when failing to write in batch mode, The function `Commit()` forgets to unlock the read-only locker, which may cause a potential deadlock. ```go func (db *NodeDatabase) Commit(node common.Hash, report bool) error { // Create a database batch to flush persistent data out. It is important that // outside code doesn't see an inconsistent state (referenced data removed from // memory cache during commit but not yet in persistent storage). This is ensured // by only uncaching existing data when the database write finalizes. db.lock.RLock() for hash, preimage := range db.preimages { if err := batch.Put(db.secureKey(hash[:]), preimage); err != nil { db.lock.RUnlock() return err } if batch.ValueSize() > xdb.IdealBatchSize { if err := batch.Write(); err != nil { return err //@audit here } batch.Reset() } } ``` Since the file is out of scope, we mark this finding as medium.

Commit: - [`0248879b845ff1a9e790580655cf7daf784536d1`](https://github.com/rangersprotocolcode/go-rangers/tree/0248879b845ff1a9e790580655cf7daf784536d1) Files: - src/core/groupchain.go When a user tries to stop a node with ctrlc, the `handleExit()` will be called. **src/gx/cli/gx.go** ```go func (gx *GX) handleExit(ctrlC <-chan bool, quit chan<- bool) { <-ctrlC fmt.Println("exiting...") consensus.StopMiner() if core.GetBlockChain() != nil { core.GetBlockChain().Close() } service.Close() middleware.Close() log.Close() if gx.init { quit <- true } else { os.Exit(0) } } ``` However, the `Close()` function from file src/core/groupchain.go which will close a leveldb `groups` won't be called. ```go func (chain *groupChain) Close() { chain.groups.Close() } ``` If you forget to close a LevelDB database properly and attempt to open it again, you may encounter an error indicating that the database is locked or in use. LevelDB uses file locking to prevent multiple processes from writing to the database simultaneously, which can cause data corruption. In addition, the `Close()` function does not close the derived leveldb `joinedGroups`.

Commit: - [`0248879b845ff1a9e790580655cf7daf784536d1`](https://github.com/rangersprotocolcode/go-rangers/tree/0248879b845ff1a9e790580655cf7daf784536d1) Files: - src/core/blockchain_add.go - src/core/sync_processor.go - src/core/sync_msg.go The `syncProcessor` struct fires periodic go-routines based on 4 internal timers that get reset each time they expire through the `Reset` call. Such method, as documented in the Go Standard Library, can be called on stopped and expired `Timer` instances once the firing channel is drained, too. In case a single handler is called upon `Timer` expiration, such guarantee is ensured by design. In case the `Timer` may be `Reset` by a different and/or concurrent handler, then a call to `Stop` should ensure the `Timer` is in a state that can be safely reset. However, in the following case such guarantee is not respected: - The `trySync` function is called not only upon the expiration of the `syncTimer`, but also in the `addCandidate` function, which is not involved in the periodic `loop` on the `syncTimer`. The consequence is that a `trySync` invocation may try to `Reset` the timer while it is still active. - The `broadcastTimer` timer is reset in `broadcastChainInfo`, but such method may be called as a go-routine in the `successOnChainCallBack` function, which is not related to the firing of the `broadcastTimer`, which can cause an inconsistent `Reset` of the timer. - In `syncProcessor.syncGroup` the `Reset` method is called on `groupReqTimer` but there is no guarantee the timer is already stopped or expired. - The `blockReqTimer` timer can be reset in the function `requestBlockChainPiece`, which will be called by the function `trySync()`. However, `trySync()` can be triggered by the timer `syncTimer`, which does not guarantee that `blockReqTimer` is stopped or expired with a drained channel. The same issue exists in the function `syncBlock`.

Commit: - [`0248879b845ff1a9e790580655cf7daf784536d1`](https://github.com/rangersprotocolcode/go-rangers/tree/0248879b845ff1a9e790580655cf7daf784536d1) Files: - src/core/blockchain.go `block` is a pointer returned from `types.UnMarshalBlock()`. However, `&block` that gets an address of a pointer(even nil value) won't be nil, thus the condition `&block == nil` won't be satisfied forever. ```go=434 var block *types.Block block, err = types.UnMarshalBlock(result) if err != nil || &block == nil { return nil } return block ```

Commit: - [`0248879b845ff1a9e790580655cf7daf784536d1`](https://github.com/rangersprotocolcode/go-rangers/tree/0248879b845ff1a9e790580655cf7daf784536d1) Files: - src/core/game_executor.go The `gameExecutor.runTransaction` function checks the `txRaw.Source` not to be an empty string. The audit team would like clarification on the cases such condition is possible.

Commit: - [`0248879b845ff1a9e790580655cf7daf784536d1`](https://github.com/rangersprotocolcode/go-rangers/tree/0248879b845ff1a9e790580655cf7daf784536d1) Files: - src/core/msg_handler.go The `ChainHandler` struct has a methods called `Handle` which has an empty implementation that returns `nil` only. The methods is called by the network module on the arrival of a new message.

Commit: - [`0248879b845ff1a9e790580655cf7daf784536d1`](https://github.com/rangersprotocolcode/go-rangers/tree/0248879b845ff1a9e790580655cf7daf784536d1) Files: - src/core/rpc_types.go The `ConvertBlockByHeader` function returns the provided block information in an `RPCBlock` struct suitable for being sent as RPC response. However, the response includes a `TransactionRoot` field which is constantly set as the empty trie root, independently from the effective list of transactions include in a block. Such practice can introduce unexpected outcomes in interaction with external components. In fact, they would obtain inconsistent information about the requested block: an empty transaction trie but a non-empty transaction list.

Commit: - [`0248879b845ff1a9e790580655cf7daf784536d1`](https://github.com/rangersprotocolcode/go-rangers/tree/0248879b845ff1a9e790580655cf7daf784536d1) Files: - go.mod The declared Golang version is `1.13`, but the `core` module contains a call to `time.Milli` which is only available starting from version `1.17`.