Map Protocol

CertiK5 Jan 2023Files named in the report

Audit

CertiK reviewed MAP Protocol on 5 Jan 2023: 73 findings, 4 critical; 67 marked fixed, 3 acknowledged, 3 open.

4 critical findings, fixed

Open the report Open MAP Protocol’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings73

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	4
High	0	3	4
Medium	0	0	5
Low	3	0	54

CriticalFree Token Mint in Function `transferInVault()` aFixed

The external function `transferInVault()` and `depositIn()` only has the modifier `checkOrder(orderId)`, in which an attacker can created unused `orderId` to bypass the check. As long as the passed `token` is a nonzero address and is in the `checkAuthToken` list, the following code will be executed: #### MapCrossChainService.sol ```solidity=213 } else if (checkAuthToken(token)) { IMAPToken(token).mint(to, amount); } ``` #### MAPCrossChainServiceRelay.sol ```solidity=370 if (checkAuthToken(token)) { MAPToken(token).mint(vaultTokenAddress, amount); } else { ``` That means the attacker can mint an arbitrary amount of MAPToken to the address `to` in the function `transferInVault()`, and to the address `vaultTokenAddress` in the function `depositIn()`.

CriticalInsecure Hash-to-Point Function Could Possibly CaFixed

With the known message $m$ and public key $pk$ provided by a validator, an attacker could possibly forge a signature to successfully verify his/her own message $m'$. Suppose a validator has the private key $sk$ to create a public key $pk = sk * g_2$, where $g_2$ is the base point of G2. The validator has the message $m$ converted to a point in G1 with the function `hashToG1()`, which uses the `keccak256()` to hash the message, that is, `keccak256(m)` and the signature is $S= sk*(keccak256(m)*g_1)$, where $g_1$ is the base point of G1. It follows the verification is passed via $$e(S, g_2) = e(sk*(keccak256(m)*g_1), g_2) = e(keccak256(m)*g_1, sk*g_2) = e(keccak256(m)*g_1, pk) = e(H(m), pk).$$ Now, suppose an attacker has message $m'$, then the attacker is able to forge the signature as $S’ = keccak256(m')* Keccak256(m)^{-1} * S$. With the validator’s known public key $pk$, the verification of the fake signature proceeds as $$e(S', g_2) = e(keccak256(m')*keccak256(m)^{-1} *sk*(keccak256(m)*g_1) , g_2) = e(keccak256(m')* sk * keccak256(m)^{-1} * keccak256(m)*g_1, g_2) = e(keccak256(m')*sk*g_1 , g_2) = e(keccak256(m')*g_1, sk*g_2) = e(keccak256(m')*g_1, pk) = e(H(m'), pk).$$ Therefore, the attacker’s message $m'$ can be verified with the original validator’s public key $pk$. In this case, the attacker can pretend to be the validator to sign any message and verify its signature. The same hash-to-point function `hashToG1()` is implemented in the atlas chain as well (https://github.com/mapprotocol/atlas/blob/main/helper/bls/bn256.go#L84-L94).

CriticalLogic Flaw In `withdraw()`Fixed

The function `withdraw()` is used to burn the user's `vtoken` to redeem the user's `correspond` token. However, after burning the portion `vtoken` of the user, the function transfers the `amount` of the `correspond` token to the current contract, which should be the other way around. ```solidity=74 _burn(msg.sender, amount); TransferHelper.safeTransferFrom(correspond, msg.sender, address(this), amount); ``` In this case, it will cause the user's loss of both `vtoken` and `correspond` tokens.

CriticalLow-level Call Will Not Be Reverted on EOAsFixed

In the contract `MapCrossChainService`, the functions `depositOutToken()` and `depositOutNative()` emit the same event `mapTransferOut(bytes token, bytes from, bytes32 orderId, uint fromChain, uint toChain, bytes to, uint amount, bytes toChainToken)`, which could possibly lead to an attack using the event from `depositOutToken()` to imitate the event from `depositOutNative()` without depositing any native token. ```solidity=192 function depositOutToken(address token, address from, address to, uint amount) external override whenNotPaused checkCanBridge(token, relayChainId) { require(msg.sender == from, "from only sender"); bytes32 orderId = getOrderID(token, from, _addressToBytes(to), amount, relayChainId); // require(IERC20(token).balanceOf(from) >= amount, "balance too low"); TransferHelper.safeTransferFrom(token, from, address(this), amount); emit mapDepositOut(token, _addressToBytes(from), orderId, selfChainId, relayChainId, to, amount); } ``` If the passed-in token address is the Externally Owned Accounts (EOA), zero address, the low-level call invoked by the `TransferHelper.safeTransferFrom()` will not be reverted. In this case, the event `mapDepositOut()` could be the same as the event from `depositOutNative()`. An attacker could pass any amount to pretend that the amount of native token has been deposited in the contract. Similarly, the functions `transferOutToken()` and `transferOutNative()` also emit the same event. ```solidity=164 function transferOutToken(address token, bytes memory toAddress, uint amount, uint toChain) external override whenNotPaused checkCanBridge(token, toChain) { require(toChain != selfChainId, "only other chain"); bytes32 orderId = getOrderID(token, msg.sender, toAddress, amount, toChain); require(IERC20(token).balanceOf(msg.sender) >= amount, "balance too low"); if (checkAuthToken(token)) { IMAPToken(token).burnFrom(msg.sender, amount); } else { TransferHelper.safeTransferFrom(token, msg.sender, address(this), amount); } emit mapTransferOut(_addressToBytes(token), _addressToBytes(msg.sender), orderId, selfChainId, toChain, toAddress, amount, _addressToBytes(address(0))); } ``` Nevertheless, the call of the method `balanceOf()` in Line 170 will be reverted if the passed-in token is an EOA. Reference:\ [`Qubit Bridge Collapse Exploited to the Tune of $80 Million`](https://certik.medium.com/qubit-bridge-collapse-exploited-to-the-tune-of-80-million-a7ab9068e1a0)

HighImproper Use of Equality CheckingFixed

The logic in the function `depositIn()` has an issue that could potentially cause the function to fail unexpectedly. In the `depositIn()` function there is an equality check made on line 365 made in situations where the input parameter `token` is a zero address. This represents ETH transfers being made into the contract. ```solidity=363 if (token == address(0)) { IWToken(wToken).deposit{value : amount}(); token == wToken; } ``` In the case of ETH transfers into the contract, the `wToken` should be used. However, in this situation, an unnecessary comparison `token == wToken` is made as `wToken` cannot be zero address. It follows that the original `token` address value (i.e., zero address) is used for the remainder of the code. Subsequent transfers within the function will fail since the `token` address will still be a zero address.

HighIncorrect Correspond Token Balance CalculationFixed

The function `withdraw()` is used to calculate the amount of correspond token needed to withdraw based on the input amount of Vtoken and burn the same amount of Vtoken at the same time. ```solidity=61 function withdraw(uint amount, address to) external override onlyManager { _burn(to, amount); correspondBalance -= getCorrespondQuantity(amount); ``` The implementation of `getCorrespondQuantity(amount)` to calculate the amount of correspond token is as follows: ```solidity=42 function getCorrespondQuantity(uint amount) public override view returns (uint){ uint allVToken = totalSupply(); if (allVToken == 0) { return amount; } return amount.mul(correspondBalance).div(allVToken); } ``` where the `allVToken` is the `totalSupply()` of the Vtoken. The function `getCorrespondQuantity()` will be invoked by the function `withdraw()` from the contract `MAPCrossChainServiceRelay` to calculate the withdrawal amount of correspond tokens. **mcs/evm/contracts/MAPCrossChainServiceRelay.sol** ```solidity=429 function withdraw(address token, uint256 vaultAmount) external { address vaultTokenAddress = feeCenter.getVaultToken(token); require(vaultTokenAddress != address(0), "only vault token"); TransferHelper.safeTransferFrom(vaultTokenAddress, msg.sender, address(this), vaultAmount); uint correspond = IVault(vaultTokenAddress).getCorrespondQuantity(vaultAmount); IVault(vaultTokenAddress).withdraw(vaultAmount, address(this)); if (token == wToken) { TransferHelper.safeWithdraw(wToken, correspond); TransferHelper.safeTransferETH(msg.sender, correspond); } else { TransferHelper.safeTransfer(token, msg.sender, correspond); } } ``` However, in the `withdaraw()` of the contract `MAPVaultToken`, it invokes the `_burn()` first, which will reduce the `totalSupply()` of Vtoken before the second calculation of correspond token. Therefore, the value `getCorrespondQuantity(amount)` will be larger than expected, which leads to the incorrect balance of correspond tokens. Consider the following scenario: 1. The vault has 100 correspond tokens and 90 Vtoken; 2. If a user stakes 10 correspond tokens, the user will be minted with 10 * 90/100 = 9 Vtoken. The total supply of Vtoken is 99 at this moment; 3. After that, the user withdraws the 9 Vtoken, the user will get 9 * 110/99 = 10 correspond tokens based on the formula of `getCorrespondQuantity(amount)`; 4. It follows that the total supply is 99 - 9 = 90 as the 9 Vtoken has been burnt; 5. The second calculation of `getCorrespondQuantity(amount)` will cause the `correspondBalance` to be reduced by 9 * 110/90 = 11 correspond tokens; 6. The correct reduced amount should be 10 correspond tokens if the burn of Vtoken occurs after the calculation; 7. In this case, future users will withdraw fewer correspond tokens as the `correspondBalance` has decreased more than expected.

HighIncorrect Decode of Near Logs to `transferOutEvenFixed

The functions `decodeNearLog()` and `decodeNearDepositLog()` are used to fetch the `nearTransferOut` and `nearDepositOut` events via the decode of the transaction logs on the Near. The `transferOutEvent` is created from the log as follows: ```solidity=536 transferOutEvent memory _outEvent = transferOutEvent({ token : logList[0].toBytes(), from : logList[1].toBytes(), order_id : bytes32(logList[2].toBytes()), from_chain : logList[3].toUint(), to_chain : logList[4].toUint(), to : logList[5].toBytes(), amount : logList[6].toUint(), to_chain_token : logList[7].toBytes() }); ``` and the `nearDepositOutEvent` is created from the log as follows: ```solidity=578 nearDepositOutEvent memory _outEvent = nearDepositOutEvent({ token : logList[0].toBytes(), from : logList[1].toBytes(), order_id : logList[2].toBytes(), from_chain : logList[3].toUint(), to_chain : logList[4].toUint(), to : logList[5].toBytes(), amount : logList[6].toUint() }); ``` While the order of both are different from the events on the Near side as shown in the file `mcs/near/map-cross-chain-service/src/event.rs`: ```rust impl Encodable for TransferOutEvent { fn rlp_append(&self, s: &mut RlpStream) { s.begin_list(8); s.append(&self.from_chain.0); s.append(&self.to_chain.0); s.append(&self.order_id.as_ref()); s.append(&self.token); s.append(&self.from); s.append(&self.to); s.append(&self.amount.0); s.append(&self.to_chain_token); } } ``` ```rust impl Encodable for DepositOutEvent { fn rlp_append(&self, s: &mut RlpStream) { s.begin_list(7); s.append(&self.from_chain.0); s.append(&self.to_chain.0); s.append(&self.order_id.as_ref()); s.append(&self.token); s.append(&self.from); s.append(&self.to); s.append(&self.amount.0); } } ``` Meanwhile, they are also different from that in the file `mcs/evmv2/contracts/utils/NearDecoder.sol`: ```solidity IEvent.transferOutEvent memory _outEvent = IEvent.transferOutEvent({ fromChain : logList[0].toUint(), toChain : logList[1].toUint(), orderId : bytes32(logList[2].toBytes()), token : logList[3].toBytes(), from : logList[4].toBytes(), to : logList[5].toBytes(), amount : logList[6].toUint(), toChainToken : logList[7].toBytes() }); ``` ```solidity IEvent.depositOutEvent memory _outEvent = IEvent.depositOutEvent({ fromChain : logList[0].toUint(), toChain : logList[1].toUint(), orderId : bytes32(logList[2].toBytes()), token : logList[3].toBytes(), from : logList[4].toBytes(), to : logList[5].toBytes(), amount : logList[6].toUint() }); ``` The inconsistent order will possibly cause an unexpected error during the decoding process.

HighPossibly Multiple Logs of Same Type in Function `Fixed

The function `decodeNearLog()` is used to obtain the `nearTransferOut` log from logs. ```solidity=501 bytes memory log; for (uint256 i = 0; i < logs.length; i++) { (bytes memory temp) = splitExtra(logs[i]); if (keccak256(temp) == nearTransferOut) { log = hexStrToBytes(logs[i]); } } ``` And the function `decodeNearDepositLog()` is used to obtain the `nearDepositOut` log from logs. ```solidity=539 bytes memory log; for (uint256 i = 0; i < logs.length; i++) { (bytes memory temp) = splitExtra(logs[i]); if (keccak256(temp) == nearDepositOut) { log = hexStrToBytes(logs[i]); } } ``` The current implementation only handles the case that there is exactly one `nearTransferOut` or `nearDepositOut` log in the logs. If there are at least two such logs, the previous ones will be overridden and only the last one will be returned.

Medium`amount` Should Be `outAmount`Fixed

The function `_transferIn()` is designed to transfer tokens to user addresses and collect fees for each transfer. The corresponding fee is calculated by function `getChainFee()`. According to the logic following `token==address(0)` and the second `else` condition, the contract only needs to transfer `outAmount` (i.e., `amount` - `fee`) tokens to users, instead of `amount` as shown in the linked code: ```solidity=334 TransferHelper.safeTransfer(token, to, amount); ``` then calls `collectChainFee()` to distribute the `fee` and transfer any leftover `fee` to fee center's contract. Additionally, the `amount` in the following line seems to be `outAmount` as well. ```solidity=333 IMAPToken(token).mint(address(this), amount); ``` On line 334, the contract will transfer `amount` tokens to the user's address, instead of `outAmount`. After going through all the if statements, the function will always call `collectChainFee()` at line 339, which will result in the fees being double counted and reduced from the total token balance within the contract. The fee balance will be incorrectly sent to the user address and also transferred out in the `collectChainFee()` function.

MediumInaccurate Sanity Check for `epoch_id`Fixed

According to the documentation of [`light node of NEAR`](https://nomicon.io/ChainSpec/LightClient), the variable `nearBlock.inner_lite.epoch_id` must be equal to the `epoch_id` or `next_epoch_id` known for the current header when updating block headers, while in the current implementation the `nearBlock.inner_lite.epoch_id` can be any value that may be a larger value than `next_epoch_id`, which may lead to an unexpected error.

Show 10 more

MediumIncorrect Number EncodingFixed

An issue with the logic pertaining to the way numbers are encoded can lead to possible manipulation by attackers. The function `getLengthenBytes()` is expected to receive a number and return its encoded representation as bytes. However, when the input number has more than 24 bits, the result will be cast or trimmed to those initial 24 bits, leading to an incorrect result for inputs with bit size within the interval between 24 to 256. As an example, consider the following smart contract: ```solidity // SPDX-License-Identifier: GPL-3.0 pragma solidity 0.8.15; import 'https://raw.githubusercontent.com/GNSPS/solidity-bytes-utils/master/contracts/BytesLib.sol'; contract EncodingExample { using BytesLib for *; function encode(uint256 num) public pure returns (bytes memory){ bytes memory result; if (num < 256){ result = abi.encodePacked(uint8(num)); } else if (num < 65536) { result = abi.encodePacked(uint16(num)); } else { result = abi.encodePacked(uint24(num)); } return result; } function equals() external pure returns (bool){ return (encode(16777215).equal(encode(33554431))); } } ``` When calling the function `encode()` with the maximum allowed `uint24` (`0xffffff`) the function will return the result `0xffffff` which is a 24 bits number. This is an expected result given the designed code. However, when the input value is larger than 2^24, for example, the decimal value 33554431, the function will also return `0xffffff` as the value. This function is used during the signature check process and given explained behavior, it could affect the signature verification process, adding malleability, and allowing an attacker to modify input parameters while leading to a fixed and verified result. Similarly, the function `getPrepareCommittedSeal()` has the same issue as the input `round` is of type uint256. It is unlikely that the round will be larger than the max of uint24, but it is recommended to add input validation to prevent unexpected results.

MediumPotential DOS AttackFixed

The `getOrderID()` function is used to generate an order id. ```solidity function getOrderID(address token, address from, bytes memory to, uint amount, uint toChainID) public returns (bytes32){ return keccak256(abi.encodePacked(nonce++, from, to, token, amount, selfChainId, toChainID)); } ``` Since `getOrderID()` is a public function, anyone can call the function at any time, and the attackers may launch a Denial-of-Service (DoS) attack via many loops to increment the `nonce` quickly. If `nonce` equals `type(uint).max`, any function calling `getOrderID()` will be reverted due to integer overflow.

MediumVariable `success` Is OverwrittenFixed

In the aforementioned lines, both function `verifyTool.vefiryHeader()` and `checkSig()` returns `success`, which leads to the first variable being overwritten. In this case, the first `success` of `verifyTool.verifyHeader()` has been ignored and only the second `success` returned by `checkSig()` will be returned, so the block validation is missing from the function `verifyHeaderSig()`.

Low`for` Loop OptimizationFixed

In function `getVerifyExpectedValueHash()`, there are multiple definitions for the same variable `_txReceipt.logs[j]` that could be optimized by the use of additional variables which help save gas and make the code easier to read.

Low`toChainToken` Could Possibly be An Unknown AddreFixed

The function `transferIn()` is designed to verify and parse the input parameter `receiptProof`, and then execute the corresponding transfer. The `_outEvent` is the event emitted on the source chain, containing the information from the source chain, such as the source chain token address and target chain address. The function `tokenRegister.getTargetToken()` serves to get the corresponding target token on the MAP Relay Chain. It will simply return zero bytes if the `_outEvent.token` has not been registered before. The returned zero bytes are directly passed into the function `_bytesToAddress()` which will return some **unknown address** that depends on the pointer to the zero bytes. ```solidity function _bytesToAddress(bytes memory bys) public pure returns (address addr){ assembly { addr := mload(add(bys, 20)) } } ``` This behavior results in the possibility that the `toChainToken` created on line 249 is an unknown address, which could possibly lead to the loss of the funds. This malicious behavior can be prevented by a `checkCanBridge` modifier at the source chain's `MAPCrossChainService.sol` to some extent. But it can still be triggered when updating the state or registering a token to the source chain, MAP Relay Chain, and target chain since the manager cannot ensure that the states of each chain are updated simultaneously.

LowDiscussion on Function `hexStrToBytes()`Fixed

The function `hexStrToBytes()` is used to convert a hex string into bytes. ```solidity=593 function hexStrToBytes(bytes memory _hexStr) internal pure returns (bytes memory) { //Check hex string is valid if ( _hexStr.length % 2 != 0 || _hexStr.length < 4 ) { revert("hexStrToBytes: invalid input"); } bytes memory bytes_array = new bytes(_hexStr.length / 2 - 32); ``` If `_hexStr.length` is less than 64, then it will pass the input validation in line 601. However, the `_hexStr.length / 2 - 32` in line 606 is negative, which leads to the function `hexStrToBytes()` being reverted. The auditing team would like to confirm with the Map team if this is the intended design that excludes the cases that `_hexStr.length` is even and `4 <= _hexStr.length < 64`.

LowFunctions with `_` as Name Prefix Are Not `privatFixed

Functions with names starting with `_` should be declared as `private`/`internal`.

LowHardcoded Validator Size InconsistencyFixed

There is a possible issue related to the inconsistency in the setting and usage of the maximum validator size. When using upgradeable smart contract patterns, it is advised not to initialize any state variables otherwise they cannot be updated using upgradeable features. In the current context, the contract `LightNode` declares the following state variable: ```solidity validator[20] public validators; ``` which is an array restricted to a maximum size of 20 validators. However additional references are found that make that declaration inconsistent: * In the `initialize()` function, the variable `maxValidators` is set to `20`. * The function `getValidators()` returns the content of the variable `maxValidators` no matter the actual size of the validator array.

LowInaccurate Event ParameterFixed

The parameter `getCorrespondQuantity(amount)` in the event `VaultWithdraw()` is not accurate as the `correspondBalance` has been updated in Line 63. ```solidity=61 function withdraw(uint amount, address to) external override onlyManager { _burn(to, amount); correspondBalance -= getCorrespondQuantity(amount); emit VaultWithdraw(getCorrespondQuantity(amount), amount); } ```

LowInaccurate Parameter of `mapTransferIn` EventFixed

According to the definition of the `mapTransferIn` event: ```solidity=218 event mapTransferIn(address indexed token, bytes indexed from, bytes32 indexed orderId, uint fromChain, uint toChain, address to, uint amount); ``` ```solidity=232 event mapTransferIn(address indexed token, bytes indexed from, bytes32 indexed orderId, uint fromChain, uint toChain, address to, uint amount); ``` The first parameter of the event is not a constant, and it is supposed to be the `token` address instead of `address(0)`.

Show 10 more

LowInaccurate Representation of `correspondBalance`Fixed

The function `correspondBalance()` uses the total token balance of the address as the total balance of `correspondToken`, however this might be an incorrect representation of the staked amount since in the case that tokens were accidentally transferred into the contract. In this case, the `correspondBalance()` would return an overstated value since it incorporates transfers that are not from user stakes.

LowInaccurate Validation of Near AccountFixed

In the aforementioned places, the two checks of the length of Near account, `to.length >= 2` || `to.length <= 64` is supposed to be the AND `&&` operator instead of the OR operator `||` based on the reference. Reference:\ https://nomicon.io/DataStructures/Account#account-id-rules

LowInconsistencies between Use of `msg.sender` and `Fixed

The use of the `from` address and `msg.sender` are not handled consistently in the function `depositOutNative()`. It does not seem as though funds are transferred on behalf of others therefore both the `from` address and `msg.sender` should be the same address. There should be a way to ensure that the two values are consistent to avoid confusion and unexpected behavior.

LowInconsistent Constant Size for Block ProducersFixed

As per the definition of the Epoch struct data model, the field `keys` are defined as ```solidity=26 struct Epoch { ... bytes32[MAX_BLOCK_PRODUCERS] keys; ... } ``` which means that maximum capacity will depend on the variable `MAX_BLOCK_PRODUCERS` defined as ```solidity=24 uint256 constant MAX_BLOCK_PRODUCERS = 100; ```

LowInconsistent Splitting LogicFixed

The function `splitExtra32()`, according to its name, should split input bytes into some sort of additional memory structure or variables. However, it is not clear how this split is done. The function is declared as: ```solidity=216 function splitExtra32(bytes memory extra) internal pure returns (bytes memory newExtra){ newExtra = new bytes(32); uint m = 0; for (uint i = 0; i < 32; i++) { newExtra[m] = extra[i]; m = m + 1; } return newExtra; } ``` According to the function body, input data is copied to a new byte array and is returned, so no split happens in the middle. Moreover, when the input data is less than 32 items the function fails due to index out-of-bound errors.

LowLack of Access Control over `doDistribute()` FuncFixed

The function `doDistribute()` transfers a predetermined amount of tokens from the caller into the addresses `vaultAddress` and `feeAddress`, while it does not enforce any access control of it.

LowLack of Checking the Required DecimalFixed

**mcs/evm/contracts/MAPCrossChainServiceRelay.sol** ```solidity function getToChainAmount(bytes memory token, uint256 fromChain, uint256 toChain, uint256 amount) public view returns (uint256){ uint256 decimalsFrom = tokenOtherChainDecimals[token][fromChain]; uint256 decimalsTo = tokenOtherChainDecimals[token][toChain]; return amount.mul(10 ** decimalsTo).div(10 ** decimalsFrom); } ``` The function `getToChainAmount()` is used to get the amount of a token on the target chain. However, if the decimal of `fromChain` or `toChain` has not been set, the `tokenOtherChainDecimals` will return 0 by default. Therefore, the `getToChainAmount()` returns the incorrect amount of the token.

LowLack of Checks for Numeric InputsFixed

Checks are missing in relation to the following numeric parameters: #### FeeCenter.sol - The following should be added to check the `proportion` in `setChainTokenGasFee()` to ensure that it is not > 10000 ```solidity require(proportion <= 10000, 'Invalid proportion value'); ``` - The following should be added to check the `rate` in `setDistributeRate()` to ensure that it is not > 10000 ```solidity require(rate <= 10000, 'Invalid rate value'); ``` - The following should be added to check if `highest` is bigger than `lowest` ```solidity require(highest >= lowest, 'Invalid highest and lowest'); ``` #### MAPCrossChainServiceRelay.sol - The following should be added to check the `rate` in `getFeeValue()` to ensure that it is not > 10000 ```solidity require(rate <= 1000000, 'Invalid rate value'); ```

LowLack of Manager Role in Some ScenariosFixed

In Solidity, when dealing with roles and permissions, it is necessary to be very cautious when removing roles from the current state to avoid potential locking scenarios. In this case, the Role contract implements functions to add and remove managers. These two operations can only be executed by the address configured at deployment with the role `DEFAULT_ADMIN_ROLE`. However, the following scenario could occur: 1. Contract is deployed and `DEFAULT_ADMIN_ROLE` role is set to address A. 2. Address A adds a new manager M1. 3. After some time, address A removes manager M1. 4. Address A account is lost. At this point, the contract functions with `onlyManager` modifier would be locked forever since no authorized address is registered for such roles.

LowMissing Ed25519 Input ValidationFixed

The given input is missing the check for data size. It is recommended to check the input data size before calling the precompiled contract. According to the Map Protocol documentation (https://docs.maplabs.io/develop/map-relay-chain/precompile-contract#map-pre-compiled-contracts), the minimum data size is 96.

Show 10 more

LowMissing Emit EventsFixed

Functions that update state variables should emit relevant events as notifications.

LowMissing Error MessagesFixed

The **require** statement 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.

LowMissing Input Validation for `chainId`Fixed

The function `transferIn()` is used to handle the `transferOutEvent` from other chains, which receives `chainId` argument as the source chain. The function `decodeNearLog()` decodes `logArray` and returns transferOutEvents as an output. ```solidity=260 function transferIn(uint256 chainId, bytes memory receiptProof) external override { (bool success,string memory message,bytes memory logArray) = lightClientManager.verifyProofData(chainId, receiptProof); require(success, message); if (chainId == ChainIdTable[1]) { (bytes memory mcsContract, transferOutEvent[] memory _outEvents) = decodeNearLog(logArray); for (uint i = 0; i < _outEvents.length; i++) { transferOutEvent memory _outEvent = _outEvents[i]; if (_outEvent.to_chain == 0){continue;} require(_checkBytes(mcsContract, bridgeAddress[chainId]), "Illegal across the chain"); ``` However, the input `chainId` may be different from `_outEvent.from_chain`, which may lead to an unexpected error.

LowMissing Input Validation for `extra.length`Fixed

In the function `splitExtra()`, the input bytes size is not validated and hence, the allocation of a new slice might fail if the length of the inputted value is smaller than 32. A `require()` statement can stop the code from failing abruptly with no explanation. ```solidity=207 newExtra = new bytes(extra.length - 32); ``` As a consequence, when variable `extra.length` is less than 32, the requested array size will be negative, creating an unexpected situation.

LowMissing Zero Address Input ValidationFixed

Address parameters should be checked before assignment or external calls to make sure they are not zero addresses. The check for the non-zero addresses is missing in the following locations: #### LightNodeProxy.sol (MapClient) ```solidity=10 constructor(address _logic, bytes memory _data) ``` - `_logic` parameter is not checked before being stored and used. #### LightNodeProxy.sol (Near) ```solidity=11 constructor(address _logic, bytes memory _data) ``` - `_logic` parameter is not checked before being stored and used. #### MAPCrossChainServiceRelay.sol ```solidity=94 function initialize(address _wToken, address _mapToken, address _managerAddress) ``` - `_wToken`, `_mapToken`, and `_managerAddress` parameters are not checked before being stored and used. ```solidity=118 function setVaultBalance(uint256 tochain, address token, uint256 amount) ``` - `token` parameter is not checked before being stored and used. ```solidity=122 function setTokenRegister(address _register) ``` - `_register` parameter is not checked before being stored and used. ```solidity=126 function setLightClientManager(address _managerAddress) ``` - `_managerAddress` parameter is not checked before being stored and used. ```solidity=158 function setFeeCenter(address fee) ``` - `fee` parameter is not checked before being stored and used. ```solidity=164 authToken[token[i]] = true; ``` - `token[i]` parameter is not checked before being stored and used. #### MapCrossChainService.sol ```solidity=75 function initialize(address _wToken, address _mapToken, address _lightNode) ``` - `_wToken`, `_mapToken`, and `_lightNode` parameters are not checked before being stored and used. ```solidity=126 function setBridge(address _bridge, uint256 _num) ``` - `_bridge` parameter is not checked before being stored and used.

LowMissing Zero Address ValidationFixed

**mcs/evm/contracts/MapCrossChainService.sol** ```solidity=110 function addAuthToken(address[] memory token) external onlyOwner { for (uint i = 0; i < token.length; i++) { authToken[token[i]] = true; } } ``` The function `addAuthToken()` is used to store the authorized tokens, which lacks a check that the input token is not a zero address that represents native token.

LowMissing Zero Address ValidationFixed

Address parameters should be checked before assignment or external calls to make sure they are not zero addresses. The check of the nonzero `receiver` address is missing in the aforementioned places.

LowPossible Inconsistency Caused by Being Hashed TwiFixed

The function `getHeaderHash()` uses the `keccak256()` to hash the block header twice. In the atlas chain, the function `signBlock()` only signs with the first hash. Aligning with the code, it seems the header has only been hashed once in the documentation. If the signature is created with the first hash, then the verification using the function `verifySign()` will fail. References: - `signBlock()`: https://github.com/mapprotocol/atlas/blob/a2749c614a52417c5ffe263a9c6cd8d02398c24d/consensus/istanbul/backend/engine.go#L557 - Documentation: https://github.com/mapprotocol/docs/blob/97e71c4ecf468693ad60e3188f55344283ef302b/develop/map-relay-chain/consensus/Overview.md

LowPossible Out-of-bound Error in Function `getValidFixed

If the `id` passed into the public function `getValidtor()` is larger than the size of the array `validators`, an unexpected error will occur.

LowPotential Division By ZeroFixed

When computing token quantity, different data sources are read to get the total supply and contract balances. However, a missing sanity check can lead to a situation in which a division by zero is triggered by the variable `allCorrespond`. ```solidity=46 return amount.mul(allVToken).div(allCorrespond); ```

Show 10 more

LowPotential Index Out of BoundFixed

In function `splitExtra32()` the inputted bytes size for variable `extra` is not validated and hence, the for loop defined in the function body will fail in situations where the function argument contains less than 32 elements.

LowPotential Index Out of BoundFixed

The following scenarios may cause the index out of range in the aforementioned lines: * In the function `decodeTxLog()`, the length of the decoded `item` is less than 3. * In the function `decodeNearLog()`, the length of the decoded `ls` is less than 2, and the length of the decoded `logList` is less than 8. * In the function `decodeNearDepositLog()`, the length of the decoded `ls` is less than 2, and the length of the decoded `logList` is less than 7.

LowPotential Logic Flaw in `getTokenFee`Fixed

The `getTokenFee()` function is used to calculate the fee of a specific token transfer. ```solidity=33 function getTokenFee(uint to, address token, uint amount) external view override returns (uint){ gasFee memory gf = chainTokenGasFee[to][token]; uint fee = amount.mul(gf.proportion).div(10000); if (fee > gf.highest){ return gf.highest; }else if (fee < gf.lowest){ return gf.lowest; } return fee; } ``` However, if the passed-in parameter `amount` for `MAPCrossChainServiceRelay`'s `transferOutToken()` function is less than `gf.lowest`, it is possible that a fee larger than the `amount` will be returned. Other functions calling the `getTokenFee()` will be reverted due to an underflow error. ```solidity function transferOutToken(address token, bytes memory to, uint256 amount, uint256 toChainId) external override whenNotPaused { //... uint256 fee = getChainFee(toChainId, token, amount); uint256 outAmount = amount.sub(fee); //... } ``` In addition, if the manager does not call function `setChainTokenGasFee()` to set the `chainTokenGasFee`, the variables `gf.proportion`, `gf.highest` and `gf.lowest` will be zero and a fee of amount zero is returned, which may allow any user to bypass the fee charge.

LowPotential Risk of Locked AdminFixed

There is a potential risk that contracts and funds will be locked because of the handling of the ownership transfer process of the admin account. This contract implements custom ownership features. The ownership management is handled by functions: * `getAdmin()` * `changeAdmin()` However, there still exists a scenario in which the contract can be run without a valid owner if the following conditions are met: * An approved owner or admin makes a transaction to update the owner address by calling function `changeAdmin()` (either willingly or tricked into doing so) * The new address for the admin is an EOA different from the previous address * Since the ownership of the EOA has not been verified by the smart contract, the EOA might be one that does not exist. As a consequence, the admin of the smart contract is now, a different EOA in which the private key is unknown. This situation will make it impossible to upgrade the smart contract or change the admin.

LowPotential Risk of Locked FundsFixed

There is a risk that funds can be locked in external contracts during upgrades due to a lack of safety measures. This contract implements an upgradeable proxy since calls are delegated to an implementation address that can be changed. This address is stored in the storage location specified by EIP-1967, so that it doesn’t conflict with the storage layout of the implementation behind the proxy. The contract initializes the upgradeable proxy with an initial implementation specified by the byte variable `_logic`. ```solidity constructor(address _logic, bytes memory _data) ```` If `_data` is non-empty, it’s used as the data in a delegate call to address `_logic`. This will typically be an encoded function call and allows initializing the storage of the proxy much like a Solidity constructor. However, the declaration of `payable` will allow the contract to also receive funds. If the delegated contract is not designed accordingly two scenarios might occur: * Funds are sent but implemented contract is not `payable`: The transaction will revert. * Funds are sent but the contract has no withdrawal/transfer/selfdestruct method: The funds will be locked in the destination contract forever.

LowPotential Signature Verification ErrorFixed

The logic pertaining to the way signatures are verified might lead to issues depending on the node environment used. The `VerifyTool` contract implements a function to verify signatures which is defined as follows: ```solidity=369 function verifySign(bytes memory seal, bytes32 hash, address coinbase) internal pure returns (bool) { (bytes32 r, bytes32 s, uint8 v) = splitSignature(seal); v = v + 27; return coinbase == ecrecover(hash, v, r, s); } ``` However, depending on the execution environment this may fail. The most likely cause of the failure to recover the signer is due to the difference in how various Ethereum implementations return the `v` portion of the signature. Different Ethereum implementations return different `v` values from the `eth_sign` call. Handling a wrong value will lead to the failure of the signature verification process. Solidity expects the `v` value to be either `27` or `28`, however other values are also possible when signing transactions using the `EIP-155` schema. Given the situation above, the client program that calls the Solidity function must supply an encoded `v` value of either `0` or `1`. However, when calling the target Ethereum node to sign, different `v` values may be returned. For example, testrpc and Ganache, both popular with development environments, return `00` or `01` for the `v` value portion of the signature. Therefore `27` must be added in order to calculate the correct value. Geth and Quorum, on the other hand, both return the `v` value according to the ecrecover technique, where the value `27` is already added.

LowRedundant CheckFixed

Since `msg.value` is equal to the `amount`, the aforementioned `require` statement is always true.

LowRedundant Import FilesFixed

The aforementioned import files are redundant and can be removed.

LowRedundant Modifier `payable`Fixed

A function that is declared `payable` can receive ether. The function `depositOutToken()` is not designed for receiving ether, so having the `payable` modifier is redundant.

LowRedundant Variable `remaining` in Function `colleFixed

The variable `remaining` is assigned as the input `amount` and has been deducted with the fee `out`, but the value has not been returned or used as the function `collectChainFee()` does not have any return value.

Show 10 more

LowRequire Condition Could be SimplifiedFixed

```sol function setDistributeRate(uint id, address to, uint rate) external onlyOwner { require(rate.add(distributeRate[0].rate).add(distributeRate[1].rate).sub(distributeRate[id].rate)<= 1000000, 'Invalid rate value'); distributeRate[id] = Rate(to, rate); } ``` The function `setDistributeRate()` is used to set distribute rates for VToken and relayer. The `require` statement in Line 73 ensures that the total distribution rate is not over 1000000. However, the required condition is too complicated to read.

LowTypos Present in CodeFixed

There were some typos found in the naming of certain functions within the contracts. #### VerifyTool.sol The function name `getBlcokHash` should be corrected to `getBlockHash`. #### IVerifyTool.sol The function name `getBlcokHash` should be corrected to `getBlockHash`. #### LightNode.sol (MapClient) The variables `validatorAddresss` and `_validatorAddresss` should be corrected to `validatorAddress` and `_validatorAddress`. The function name `getValidatorsIdPrve` should be corrected to something like `getValidatorsIdPrev`. The function name `getLengthenBytes` should be corrected to something like `getLengthInBytes`. #### LightNode.sol (Near) The function name `trigglePause` should be corrected to something like `togglePause`. The error message `failed or unknow transation` should be corrected to `failed or unknown transaction`. The function name `getEcopKeys()` should be corrected to `getEpochKeys()` #### ILightNode.sol (MapClient) The state variable `validaters` should be corrected to `validators`. #### MAPCrossChainServiceRelay.sol The function name `setBridageAddress` should be corrected to `setBridgeAddress`. The variable name `sucess` should be corrected to `success`. #### MapCrossChainService.sol The error message `token not can bridge` should be corrected to something like `token cannot be bridged`. The variable name `sucess` should be corrected to `success`. #### IWeightedMultiSig.sol The function name `upateValidators` should be corrected to `updateValidators`. #### TransferHelper.sol The error message `TransferHelper: WiTHDRAW_FAILED` should be corrected to `TransferHelper: WITHDRAW_FAILED`.

LowUnimplemented `transferOut()` FunctionsFixed

The `transferOut()` function in both `MapCrossChainService.sol` and `MAPCrossChainServiceRelay.sol` contracts are not implemented.

LowUnlocked Compiler VersionFixed

The contract has unlocked compiler version. An unlocked compiler version in the source code of the contract permits the user to compile it at or above a particular version. This, in turn, leads to differences in the generated bytecode between compilations due to differing compiler version numbers. This can lead to an 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.

LowUnnecessary Length CheckFixed

In certain areas, the code can be optimized by reducing redundant checking criteria. In function `decodeExtraData()` the contract performs several operations to decode input memory bytes to an `RLPItem`. During the process, three for loops are defined. Consider the following example: ```solidity=102 if (item0.length > 0) { for (uint256 i = 0; i < item0.length; i++) { validatorTemp[i] = item0[i].toAddress(); } } ``` Note that `item0.length > 0` is equal to the loop condition `i < item0.length`. Since the main logic of the block is to execute the loop over all input elements, the code can be optimized by removing the first conditional statement.

LowUnnecessary Variable UsageFixed

In function `splitExtra32()`, the use of variable `m` can be replaced by variable `i` from the for loop statement. There is no need to use an extra variable that behaves exactly as loop counter.

LowUnused Variable `vaultBalance` and Function `setFixed

The state variable `vaultBalance` is declared and can be modified by the function `setVaultBalance()`, but none of them are used in the contract.

LowUnused EventsFixed

#### Unused Events The following events are declared but not used anywhere in the contract code. ```solidity=61 event mapTransferOutData(bytes indexed toContract, address indexed from, bytes32 indexed orderId, uint fromChain, uint toChain, bytes data); event mapTransferInData(bytes indexed toContract, address indexed from, bytes32 indexed orderId, uint fromChain, uint toChain, bytes data); ```

LowUnused Function `setVaultValue()`Fixed

All the statements that call the function `setVaultValue()` are commented out and the internal function `setVaultValue()` is unreachable.

LowUnused InterfacesFixed

There are interface contracts present in the codebase that are never referenced by any contract.

Show 7 more

LowUnused State VariablesFixed

One or more state variables are never used in the codebase. The following are unused variables in various contracts: #### MAPVaultToken.sol * accrualBlockNumber * userStakingAmount * correspondToken #### NearDecoder.sol * VALIDATOR_V1 #### MapCrossChainService.sol * tokenRegister * chainGasFees #### BGLS.sol * pminus * pplus #### FeeCenter.sol * chainId

LowUsage of `block.chainId` Preferred over AssemblyFixed

The `block.chainid` function is available in native solidity since 0.8.0. Its usage is recommended to avoid assembly misconfigurations and to improve code readability.

LowUsage of Magic NumbersFixed

For example, in the function `deleteSealAndAgg()`, the following magic numbers are used in many locations instead of constant values. ```solidity=340 list[4] = new bytes(1); list[4][0] = bytes1(uint8(128)); list[5] = new bytes(4); list[5][0] = bytes1(uint8(195)); list[5][1] = bytes1(uint8(128)); list[5][2] = bytes1(uint8(128)); list[5][3] = bytes1(uint8(128)); ``` As a consequence, it reduces the code readability and becomes difficult to understand the purpose of these values. A similar issue is also present in the function `deleteAgg()`.

LowUse Variable of `ls[1].toList()` for Gas OptimizaFixed

The function `decodeNearLog()` is used to decode RLPItems from the input `logsHash`. **mcs/evm/contracts/MAPCrossChainServiceRelay.sol** ```solidity=487 function decodeNearLog(bytes memory logsHash) internal view returns (bytes memory executorId, transferOutEvent memory _outEvent){ RLPReader.RLPItem[] memory ls = logsHash.toRlpItem().toList(); executorId = ls[0].toBytes(); bytes[] memory logs = new bytes[](ls[1].toList().length); for (uint256 i = 0; i < ls[1].toList().length; i++) { logs[i] = ls[1].toList()[i].toBytes(); } ``` The function `toList()` is used to decode the RLPItem list from `ls[1]`. **mcs/evm/contracts/utils/RLPReader.sol** ```solidity=117 function toList(RLPItem memory item) internal pure returns (RLPItem[] memory) { require(isList(item), "is list fail"); uint256 items = numItems(item); RLPItem[] memory result = new RLPItem[](items); uint256 memPtr = item.memPtr + _payloadOffset(item.memPtr); uint256 dataLen; for (uint256 i = 0; i < items; i++) { dataLen = _itemLength(memPtr); result[i] = RLPItem(dataLen, memPtr); memPtr = memPtr + dataLen; } return result; } ``` Every time the loop statement enters into the condition and body, the function `ls[1].toList()` will be invoked, which costs a lot of gas.

LowUse Variables of `ls[i].toList()` and `ls[i].toLFixed

The function `decodeTxLog()` is used to decode RLPItems from the input `logsHash`. **mos/evm/contracts/MAPCrossChainService.sol** ```solidity=262 function decodeTxLog(bytes memory logsHash) internal pure returns (txLog[] memory _txLogs){ RLPReader.RLPItem[] memory ls = logsHash.toRlpItem().toList(); _txLogs = new txLog[](ls.length); for (uint256 i = 0; i < ls.length; i++) { bytes[] memory topic = new bytes[](ls[i].toList()[1].toList().length); for (uint256 j = 0; j < ls[i].toList()[1].toList().length; j++) { topic[j] = ls[i].toList()[1].toList()[j].toBytes(); } _txLogs[i] = txLog({ addr : ls[i].toList()[0].toAddress(), topics : topic, data : ls[i].toList()[2].toBytes() }); } } ``` The function `toList()` is used to decode the RLPItem list from `ls[i]` and `ls[i].toList()[1]`. **mos/evm/contracts/utils/RLPReader.sol** ```solidity=117 function toList(RLPItem memory item) internal pure returns (RLPItem[] memory) { require(isList(item), "is list fail"); uint256 items = numItems(item); RLPItem[] memory result = new RLPItem[](items); uint256 memPtr = item.memPtr + _payloadOffset(item.memPtr); uint256 dataLen; for (uint256 i = 0; i < items; i++) { dataLen = _itemLength(memPtr); result[i] = RLPItem(dataLen, memPtr); memPtr = memPtr + dataLen; } return result; } ``` Every time the loop statement enters into the condition and body, the function `ls[i].toList()` and `ls[i].toList()[1].toList()` will be invoked, which cost a lot of gas.

LowUse Variables to Save GasFixed

When coding multiple statements that contain the same logic, intermediate variables can be used to save gas during the execution of the algorithm. In the `VerifyTool` contract, the function `decodeTxLog()` can benefit from variable usage. Consider the following code: ```solidity=157 function decodeTxLog(bytes memory logsHash) public pure returns (txLog[] memory _txLogs){ RLPReader.RLPItem[] memory ls = logsHash.toRlpItem().toList(); _txLogs = new txLog[](ls.length); for(uint256 i = 0; i< ls.length; i++){ bytes[] memory topic = new bytes[](ls[i].toList()[1].toList().length); for(uint256 j = 0; j < ls[i].toList()[1].toList().length; j++){ topic[j] = ls[i].toList()[1].toList()[j].toBytes(); } _txLogs[i] = txLog({ addr:ls[i].toList()[0].toAddress(), topics : topic, data : ls[i].toList()[2].toBytes() }); } } ``` Note that `ls[i].toList()`is duplicated 5 times and `ls[i].toList()[1].toList()`is duplicated 3 times. The same function is also included in the `MAPCrossChainServiceRelay` contract.

LowVariable Can Be Declared As `constant`Fixed

To save gas following variables can be declared as constant. ```solidity=11 bytes4 fun = bytes4(keccak256(bytes('initialize(address,address)'))); ``` Additionally, the signature value `0x485cc955` can be inline as a replacement for the `keccak256` function call.