Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added m0 report #79

Merged
merged 1 commit into from
Dec 20, 2024
Merged

added m0 report #79

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
207 changes: 207 additions & 0 deletions reports/Cyfrin/2024-11-26-cyfrin-m0-v2.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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