Rangers Protocol security record

The code mistakenly uses a comparison operator instead of the intended assignment operator. Potentially allowing a signature to be used multiple times. Example line of code: ```solidity=1634 g_Workedmap[nonce] == true; ```

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.

`WithdrawErc20_CM` accepts `_tocontract` as an address, while `WithdrawErc20_CM2` accepts `_tocontract` as bytes. However, during the signature encoding process, an address is converted to bytes. This conversion allows the parameters of WithdrawErc20_CM2 to pass validation in WithdrawErc20_CM. The function `WithdrawErc20_CM2` has the restrictions on the `fromchainid`, but the `WithdrawErc20_CM` does not have. The parameters for the function `WithdrawErc20_CM2` can be called successfully in the `WithdrawErc20_CM`.

The `WithdrawErc721_CM` function relies on the `ownerOf` method of the ERC721 contract to determine the owner of a given tokenId. ```solidity /** * @dev Returns the owner of the `tokenId` token. * * Requirements: * * - `tokenId` must exist. */ function ownerOf(uint256 tokenId) external view returns (address owner); ``` However, this approach introduces a vulnerability and may block minting operations due to the following issues: If the `tokenId` does not exist or the `owner` is not the `address(this)`, both scenarios result in `ok` being `false`. However, the minting logic should differentiate between these two cases to ensure proper handling: - If the tokenId does not exist: The contract should mint the token. - If the tokenId exists but the owner is not address(this): The contract is unable to mint the token. ```solidity bool ok; try g_CrossEx.TryownerOf(_fromcontract,_nftid,address(this)) returns(bool v) { ok = v; }catch{ ok = false; } if(ok == true){ IERC721(_fromcontract).transferFrom(address(this),_addr,_nftid); g_Workedmap[nonce] == true; emit event_withdrawErc721_CM(_fromcontract,_fromchainId,_tocontract,_addr,_nftid); return; }else{ IERC721(_fromcontract).mint(_addr,_nftid); g_Workedmap[nonce] == true; emit event_withdrawErc721_CM(_fromcontract,_fromchainId,_tocontract,_addr,_nftid); return; } ```

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`.

The function `transfer()` in the contract `RPG.sol` added limit for the sender account and receipt account. However, the function `transferFrom()` has no similar limit.

The `WithdrawErc721_CM` function should call the commented `VerifyCrossErc721Sign` function rather than the `VerifyCrossErc20Sign`. Although the functionality of these two functions are same, the naming convention is bad.