diff --git a/reports/Cyfrin/2024-11-26-cyfrin-m0-v2.0.md b/reports/Cyfrin/2024-11-26-cyfrin-m0-v2.0.md new file mode 100644 index 0000000..a99f128 --- /dev/null +++ b/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 \ No newline at end of file