MAP Protocol security record

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

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

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.

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)

The BLS signature scheme requires a cryptographic hash function that maps arbitrary data to a point in one of the two groups of the chosen elliptic curve. Such a function is also required to respect the properties of the Random Oracle Model. The `hash_to_g1` function at the pointed location maps arbitrary data to a point in the $G1$ group. Its implementation first performs a `sha256` on the input data and then ec-multiplies the resulting string with the generator of the $G1$ group. This is a known insecure manner to implement an hash-to-point function. In fact, the presence of the generator of the $G1$ group inside such expression allows, given a key pair (sk, PK) and a legitimate signature `S` of a message $m$ created by the user holding `sk`, an attacker to forge a signature `S'` of any arbitrary message `m'` such that `verify(S', m', PK) = true` having the sole knowledge of $S$ and $m$. In respect to the audited logic, this vulnerability allows an attacker to forge the aggregated signature carried by validated blocks. Indeed, each valid block, that is public, includes a signature and the date that the signature is supposed to authenticate. This means that any attacker, including malicious Map Relay Chain validator or Maintainers, can let the light client be updated with any fake block containing arbitrary data.

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.

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.

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.

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.

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.