Rangersprotocol-CrossContract

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

`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; } ```

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.

The **require** can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller.

The functions of `DoCross1155_CM` exist in both `CrossContract` and `CrossContractEx`: # Function Name Shadowing: Both `CrossContract` and `CrossContractEx` define a function with the same name (DoCross1155_CM), leading to potential confusion and reduced readability. The sanem shadowing issue applies to the `WithdrawErc1155_CM` function. Name shadowing could result in developers or auditors misinterpreting the flow or function call source, increasing the likelihood of errors. ```solidity contract CrossContract{ CrossContractEx public g_CrossEx; function DoCross1155_CM(address _fromcontract,address _tocontract,uint256 _toChainId,address _fromaddr,address _toaddr,uint256[] memory _ids,uint256[] memory _values) payable external{ {... } } contract CrossContractEx is Ownable{ function DoCross1155_CM(address sender,address _fromcontract,address _fromaddr,uint256[] memory _ids,uint256[] memory _values) payable external returns(bool){ ... } } ```

The `CrossContractEx` has at least one payable function that does not utilize the forwarded ETH. Additionally, these contracts lack a mechanism to withdraw ETH. As a result, any ETH sent to these contracts may become permanently trapped.