Merge pull request #79 from 0ximmeas/main

added m0 report

reports/Cyfrin/2024-11-26-cyfrin-m0-v2.0.md

@@ -0,0 +1,207 @@
+**Lead Auditors**
+
+[0kage](https://twitter.com/0kage_eth)
+
+[Immeas](https://twitter.com/0ximmeas)
+
+**Assisting Auditors**
+
+
+
+---
+
+# Findings
+## Medium Risk
+
+
+### Cross-chain token transfer from `SpokeVault` fails due to approval from implementation contract instead of proxy
+
+**Description:** [`SpokeVault`](https://github.com/m0-foundation/m-portal/blob/8f909076f4fc1f22028cabd6df8a9e6b5be4b078/src/SpokeVault.sol) is a contract that holds excess M tokens from Smart M and can transfer this excess to `HubVault` on mainnet via [`SpokeVault::transferExcessM`](https://github.com/m0-foundation/m-portal/blob/8f909076f4fc1f22028cabd6df8a9e6b5be4b078/src/SpokeVault.sol#L69-L89).
+
+To enable this, an infinite approval is set in the [`SpokeVault` constructor](https://github.com/m0-foundation/m-portal/blob/8f909076f4fc1f22028cabd6df8a9e6b5be4b078/src/SpokeVault.sol#L63-L64):
+```solidity
+// Approve the SpokePortal to transfer M tokens.
+IERC20(mToken).approve(spokePortal_, type(uint256).max);
+```
+
+However, `SpokeVault` is deployed as a proxy, as seen in [`DeployBase::_deploySpokeVault`](https://github.com/m0-foundation/m-portal/blob/8f909076f4fc1f22028cabd6df8a9e6b5be4b078/script/deploy/DeployBase.sol#L221-L235):
+```solidity
+spokeVaultImplementation_ = address(
+    new SpokeVault(spokePortal_, hubVault_, destinationChainId_, migrationAdmin_)
+);
+
+spokeVaultProxy_ = _deployCreate3Proxy(address(spokeVaultImplementation_), _computeSalt(deployer_, "Vault"));
+```
+Where [`_deployCreate3Proxy`](https://github.com/m0-foundation/m-portal/blob/8f909076f4fc1f22028cabd6df8a9e6b5be4b078/script/helpers/Utils.sol#L102-L108) deploys an `ERC1967Proxy`.
+
+As a result, the approval set in the constructor applies only to the implementation contract, not to the proxy, which holds the tokens and initiates the transfers.
+
+**Impact:** `SpokeVault::transferExcessM` will not function as intended because the `Portal` requires approval to transfer tokens. The M tokens in `SpokeVault` will be effectively stuck, though not permanently, as the contract is upgradeable.
+
+**Proof of Concept:** Adding a token transfer in [`MockSpokePortal::transfer`](https://github.com/m0-foundation/m-portal/blob/8f909076f4fc1f22028cabd6df8a9e6b5be4b078/test/mocks/MockSpokePortal.sol#L14-L21) will cause [`SpokeVaultTests::test_transferExcessM`](https://github.com/m0-foundation/m-portal/blob/8f909076f4fc1f22028cabd6df8a9e6b5be4b078/test/unit/Spokevault.t.sol#L115-L138) to fail:
+```diff
++import { IERC20 } from "../../lib/common/src/interfaces/IERC20.sol";
+...
+    function transfer(
+        uint256 amount,
+        uint16 recipientChain,
+        bytes32 recipient,
+        bytes32 refundAddress,
+        bool shouldQueue,
+        bytes memory transceiverInstructions
+    ) external payable returns (uint64) {
++       IERC20(mToken).transferFrom(msg.sender, address(this), amount);
+    }
+```
+
+**Recommended Mitigation:** Consider setting token approvals only as needed in `transferExcessM` and removing the approval from the constructor:
+```diff
++ IERC20(mToken).approve(spokePortal_, amount_);
+  messageSequence_ = INttManager(spokePortal).transfer{ value: msg.value }(
+      amount_,
+      destinationChainId,
+      hubVault_,
+      refundAddress_,
+      false,
+      new bytes(1)
+   );
+```
+In addition, this approach eliminates the infinite approval to an upgradeable contract, enhancing security.
+
+**M0 Foundation**
+Fixed in commit [78ac49b](https://github.com/m0-foundation/m-portal/commit/78ac49bba3ac294873a949eec00a5bfad8b41c34)
+
+**Cyfrin**
+Verified
+
+
+### SpokeVault transfers will frequently revert due to Wormhole gas fee fluctuations
+
+**Description:** The `SpokeVault.transferExcessM()` function forwards ETH to pay for Wormhole gas fees, but any excess ETH sent will cause the transaction to revert since `SpokeVault` lacks capability to receive ETH refunds. Current implementation only works if the user sends a fee that is exactly equal to the  wormhole gas fee at the time of transaction.
+
+This creates a significant usability problem because Wormhole gas fees can fluctuate between the time a user calculates the fee off-chain and when their transaction is actually executed.
+
+The following code in `ManagerBase::_prepareForTransfer` shows that any gas fee shortfall will revert the transaction. Also, any excess gas fee over and above the delivery fee is refunded back to the sender.
+
+```solidity
+// In ManagerBase.sol
+function _prepareForTransfer(...) internal returns (...) {
+    // ...
+    if (msg.value < totalPriceQuote) {
+        revert DeliveryPaymentTooLow(totalPriceQuote, msg.value);
+    }
+
+    uint256 excessValue = msg.value - totalPriceQuote;
+    if (excessValue > 0) {
+        _refundToSender(excessValue); // Reverts as SpokeVault can't accept ETH
+    }
+}
+
+  function _refundToSender(
+        uint256 refundAmount
+    ) internal {
+        // refund the price quote back to sender
+        (bool refundSuccessful,) = payable(msg.sender).call{value: refundAmount}("");
+         //@audit excess gas fee sent back to msg.sender (SpokeVault)
+
+        // check success
+        if (!refundSuccessful) {
+            revert RefundFailed(refundAmount);
+        }
+    }
+```
+
+
+As a result, `transferExcessM` can only run if the user sends the exact fee. Doing so would be challenging because:
+
+- Since this function can be called by anyone, it is likely that an average user would not know how to calculate the delivery fees
+- Even if a user calculates the exact fee off-chain, it is highly likely that transaction fails due to natural gas fee fluctuation
+
+
+**Impact:** If gas fee increases or decreases even slightly between quote and execution, transaction reverts.
+
+**Proof of Concept**
+Make following changes to `MockSpokePortal` and run the test below in `SpokeVault.t.sol`:
+
+```solidity
+      contract MockSpokePortal {
+          address public immutable mToken;
+          address public immutable registrar;
+
+          constructor(address mToken_, address registrar_) {
+              mToken = mToken_;
+              registrar = registrar_;
+          }
+
+          function transfer(
+              uint256 amount,
+              uint16 recipientChain,
+              bytes32 recipient,
+              bytes32 refundAddress,
+              bool shouldQueue,
+              bytes memory transceiverInstructions
+          ) external payable returns (uint64) {
+
+              // mock return of excess fee back to sender
+              if(msg.value > 1) {
+                  payable(msg.sender).transfer(msg.value-1);
+              }
+
+          }
+      }
+
+   contract SpokeVaultTests is UnitTestBase {
+        function testFail_transferExcessM() external { //@audits fails with excess fee
+            uint256 amount_ = 1_000e6;
+            uint256 balance_ = 10_000e6;
+            uint256 fee_ = 2;
+            _mToken.mint(address(_vault), balance_);
+            vm.deal(_alice, fee_);
+
+            vm.prank(_alice);
+            _vault.transferExcessM{ value: fee_ }(amount_, _alice.toBytes32());
+        }
+   }
+
+```
+
+**Recommended Mitigation:** Wormhole has a specific provision to refund excess gas fee back to the sender. This is put in place to ensure reliability of transfers even with natural gas fee fluctuations.
+
+Consider updating the `SpokeVault.transferExcessM()` function to handle excess ETH refunds by adding a `payable receive()` function in `SpokeVault`. Also, please make sure the logic forwards any received ETH back to the original caller - not doing so would result in the excess gas fee stuck inside `SpokeVault`.
+
+**M0 Foundation**
+Fixed in commit [78ac49b](https://github.com/m0-foundation/m-portal/commit/78ac49bba3ac294873a949eec00a5bfad8b41c34)
+
+**Cyfrin**
+Verified
+
+\clearpage
+## Informational
+
+
+### `ExcessMTokenSent` event logs `messageSequence_` as `0` incorrectly
+
+**Description:** When calling [`SpokeVault::transferExcessM`](https://github.com/m0-foundation/m-portal/blob/8f909076f4fc1f22028cabd6df8a9e6b5be4b078/src/SpokeVault.sol#L69-L89) to transfer excess M tokens from `SpokeVault`, an `ExcessMTokenSent` event is emitted:
+```solidity
+) external payable returns (uint64 messageSequence_) {
+    ...
+
+    // @audit messageSequence_ not assigned yet
+    emit ExcessMTokenSent(destinationChainId, messageSequence_, msg.sender.toBytes32(), hubVault_, amount_);
+
+    messageSequence_ = INttManager(spokePortal).transfer{ value: msg.value }(
+        ...
+    );
+}
+```
+However, at the time the event is emitted, the `messageSequence_` value hasn’t been assigned and will therefore always be 0.
+
+**Recommended Mitigation:** Consider emitting the event after the `transfer` call, once `messageSequence_` has been assigned a valid value.
+
+**M0 Foundation**
+Fixed in commit [78ac49b](https://github.com/m0-foundation/m-portal/commit/78ac49bba3ac294873a949eec00a5bfad8b41c34)
+
+**Cyfrin**
+Verified
+
+\clearpage