WeMix - bridge audit 1

The signature verification process doesn't check for signature reuse. A malicious validator or an attacker-compromised validator could bypass the `validatedsignum` check by using the same signatures repeatedly to reach the required signature amount. ```solidity uint sigNum = SafeMath.div(signatures.length, 65); uint validatedSigNum; bytes32 id_ = id; address ca_ = address(this); address tokenAddr_ = tokenAddr; address from_ = from; address payable to_ = to; bytes32 fromChain_ = fromChain; bytes32 txHash_ = txHash; uint amount_ = amount; // multi sig validation for(uint i = 0; i < sigNum; i++){ bytes calldata sig = signatures[i*65:(i+1)*65]; bytes memory message = abi.encodePacked(id_, ca_, tokenAddr_, from_, to_, fromChain_, txHash_, amount_); if(_validatorRole.isValidatorSig(message, sig) == true){ validatedSigNum++; } } ``` ***Exploit Scenario:*** Let's say there are 5 validators (A-E) in total. The attacker manages to compromise validator A and forge a signature `s`. Then he sets the @param `signatures` as 3 duplicated `s`. The layout of @param `signatures` is like the following: ```rust s||s||s ``` The forged signatures can still pass the valid signatures number check in L303, although there is only one valid signature. ```solidity=303 require(validatedSigNum > SafeMath.div(_validatorRole.numberValidators(),2), "WemixBridge: fail to satisfy sufficient number of valid sig"); ```

There are no checks in the function `exit()` that can make sure @param `from` is the `msg.sender`. Function caller can set @param `from` as arbitrary addresses. Since users have to approve the bridge contract for exchanging tokens, anyone can call this function to burn/transfer tokens out of the user addresses: ```solidity= if(Tokens.Crypto(_wemixBridgeStorage.getTokenCryptoType(id)) == Tokens.Crypto.COIN) { require(msg.value == SafeMath.add(amount,feeAmount), "WemixBridge: not enoungh coin"); _payCost(service, address(0), feeAmount, from, _INCOME_EXCHANGE); }else{ if(_wemixBridgeStorage.getTokenMintable(id)){ require(IWemixBridgeERC20(_wemixBridgeStorage.getTokenAddr(id)).burnFrom(from, amount), "WemixBridge: failed to burn token"); //burn }else{ require(IERC20(_wemixBridgeStorage.getTokenAddr(id)).transferFrom(from, address(this), amount), "WemixBridge: failed to transferFrom token"); //deposit to this contract } _payCost(service, tokenAddr, feeAmount, from, _INCOME_EXCHANGE); } ``` Missing checks on @param `from` may cause loss of users' assets and other severe consequences.

The linked codes are not compatible with solc version v0.6.0 ```solidity= function enter( bytes32 id, address tokenAddr, address from, address payable to, bytes32 fromChain, bytes32 txHash, uint amount, bytes calldata signatures ) ``` The contracts cited have an unlocked compiler version. An unlocked compiler version in the source code of the contract permits the user to compile it at or above version v0.6.0. This, in turn, leads to differences in the generated bytecode between compilations due to differing compiler version numbers. This can lead to ambiguity when debugging, as compiler-specific bugs may occur in the codebase that would be hard to identify over a span of multiple compiler versions rather than a specific one. ***Proof of concept:*** Following is the error message when we try to use compiler version 0.6.0: ```Solidity TypeError: Data location must be "memory" for parameter in function, but "calldata" was given. ```

In the current implementation of contract `MinterRoleV2`, @param `bearersArray` is not updated in function `addMinter()`. While in the function `removeMinter`, operation `Array.pop` is performed on the empty `bearersArray`, which causes an exception. The following error will be shown when the function is invoked: ```bash VMError (if there is one): VMError { │status false Transaction mined but execution failed │transaction hash 0x78cd181cfe80530c035a111d5e933a7e344d54e567701891b33f33a46d2fd12c │from 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4 │to MinterRoleV2.removeMinter(address) 0xd9145CCE52D386f254917e481eB44e9943F39138 │gas 3000000 gas │transaction cost 3000000 gas │execution cost 3000000 gas │input 0x309...eddc4 │decoded input { │ "address account": "0x5B38Da6a701c568545dCfcB03FcB875f56beddC4" │} │decoded output {} │logs [] │val 0 wei │transact to MinterRoleV2.removeMinter errored: VM error: invalid opcode. │ │invalid opcode │ │ The execution might have thrown. │} ``` The error will lead to the function `removeMinter()` being inaccessible.

The function `lockToken()` requires the token to be unlocked, and accordingly, the function `unlockToken()` should require the token to be locked. ```solidity=193 require(_wemixBridgeStorage.getTokenLock(id) == Tokens.Lock.UNLOCK, "WemixBridge: token is already unlocked"); ```

The linked code can be moved outside of the loop to save gas. ```solidity=295 bytes memory message = abi.encodePacked(id_, ca_, tokenAddr_, from_, to_, fromChain_, txHash_, amount_); ```

In the public function `exit`, there is no sanity check on function parameter `_tochain`. ```solidity=200 function exit( bytes32 service, bytes32 id, address tokenAddr, address from, address to, bytes32 toChain, uint amount, uint feeAmount ) public payable whenNotPaused nonReentrant { require(id != bytes32(0), "WemixBridge: invalid token id"); require(_wemixBridgeStorage.getTokenId(id) == id, "WemixBridge: incorrect token id"); require(_wemixBridgeStorage.getTokenAddr(id) == tokenAddr, "WemixBridge: incorrect token address"); require(!_wemixBridgeStorage.getTokenLocked(id), "WemixBridge: token is locked"); require(amount > 0, "WemixBridge: amount must greater than zero"); ``` Parameter `_toChain` can be set as any chain name, including the current deployed chain `_chain`. Invalid chain names might cause the failure of bridge exit operation and the loss of user's asset.

The `isOwner()` check in the function `isValidatorSig()` has already been performed in the function `isValidator()` and thus is redundant. ```solidity=28 function isValidator(address account) public view override returns (bool) { return (bearersMap[account] == true || isOwner()); } function isValidatorSig(bytes memory message, bytes memory signature) public view override returns (bool){ address recover = keccak256(message).recover(signature); return isValidator(recover) == true || isOwner(); } ```