diff --git a/audits/Kirill-Fedoseev-audit-report-v1.md b/audits/Kirill-Fedoseev-audit-report-v1.md index 6d9dc41..5f63a11 100644 --- a/audits/Kirill-Fedoseev-audit-report-v1.md +++ b/audits/Kirill-Fedoseev-audit-report-v1.md @@ -1,13 +1,13 @@ -# M^0 Usual Smart M Extension Security Review +# M^0 M Portal Security Review -Date: **22.11.24** +Date: **12.11.24** Produced by **Kirill Fedoseev** (telegram: [kfedoseev](http://t.me/kfedoseev), twitter: [@k1rill_fedoseev](http://twitter.com/k1rill_fedoseev)) ## Introduction -An independent security review of the M^0 Usual Smart M Extension contracts was conducted by **kfedoseev** on 22.11.24. +An independent security review of the M^0 M Portal contracts was conducted by **kfedoseev** from 07.11.24 to 12.11.24. The following methods were used for conducting a security review: - Manual source code review @@ -18,21 +18,24 @@ No security review can guarantee or verify the absence of vulnerabilities. This where I tried to identify as many potential issues and vulnerabilities as possible, using my personal expertise in the smart contract development and review. -## About M^0 Usual Smart M Extension +## About M^0 M Portal -Usual Smart M Extension is a semi-permissioned ERC20 wrapper of Smart M token, designed to be used as collateral for -generating USD0. +M Portal is a set of contracts built on top of the Wormhole NTT (Native Token Transfers) framework, designed to +facilitate bridging of M token between Ethereum Mainnet and various other chains. ## Observations and Limitations -* The `UsualM` contract is upgradeable, with upgradeability being managed by a multi-sig. -* `UsualM` wrapping is permissionless, however, unwrapping is permissioned and controlled by the Usual's - `RegistryAccess` contract. `UsualM` also contains pausing and blacklisting functionality controlled by the roles - defined in the Usual's `RegistryAccess` contract. -* Management and usage of the `UsualM` contract as well as all Smart M funds wrapped in `UsualM` are delegated to the - Usual protocol and its smart contracts, whose security hasn't been evaluated as part of this review. From the Smart M - perspective, `UsualM` is yet another user of the Smart M token with no additional permissions, thus, in the worst case - scenario, regardless of `UsualM` contract behavior, other `Smart M` or `M` users won't be impacted in any way. +* `HubPortal`, `SpokePortal` and `SpokeVault` contracts are upgradeable via the TTG governance. Registrar key-value + pairs controlling the upgrade process can be directly altered through **StandardGovernor** or **EmergencyGovernor**, + or indirectly through the **ZeroGovernor**. For `SpokeVault` and `SpokePortal`, registrar key values need to be + additionally pushed through the Wormhole bridge using the `HubPortal`. Additionally, contracts can be upgraded by a + migration admin (multi-sig), bypassing the governance upgradability flow. Upgradability is controlled by the + implementation contract and can be forfeited in the future as part of one of the upgrades. +* Security of all funds stored in `HubPortal` is based on the underlying Wormhole bridge, whose security hasn't been + evaluated as part of this review. In the worst case scenario, if the Wormhole bridge is compromised for one or more + connected chains, all funds in the `HubPortal` are at risk, regardless of the total M supply on the given L2. +* While Wormhole bridge supports non-EVM chains, the current M Portal version targets only EVM-based chains. Forward + compatibility with any non-EVM chains has not been assessed as part of the review. ## Severity classification @@ -52,70 +55,234 @@ generating USD0. Reviewed commits: -* M Extensions - - [7ed8340dcfdca31ae24c4d2ceca3bebb47798b04](https://github.com/m0-foundation/m-extensions/commit/7ed8340dcfdca31ae24c4d2ceca3bebb47798b04) +* M Portal - + [fd059a9f2a02d4c7522df19b1cf32f3d5ee45290](https://github.com/m0-foundation/m-portal/commit/fd059a9f2a02d4c7522df19b1cf32f3d5ee45290) +* Common - + [3692db150ad90b21d7c213ea535f34792ad8873f](https://github.com/m0-foundation/common/commit/3692db150ad90b21d7c213ea535f34792ad8873f) +* Protocol (`spoke` branch) - + [9661cce9b3562a7c17afc8b60dddf216c59de1f3](https://github.com/m0-foundation/protocol/commit/9661cce9b3562a7c17afc8b60dddf216c59de1f3) +* TTG (`spoke` branch) - + [2a11c24f88f4b0b31e376dc61b73ab8a9a67190a](https://github.com/m0-foundation/ttg/commit/2a11c24f88f4b0b31e376dc61b73ab8a9a67190a) Reviewed contracts: -- `src/oracle/AggregatorV3Interface.sol` -- `src/oracle/NAVProxyMPriceFeed.sol` -- `src/usual/interfaces/IRegistryAccess.sol` -- `src/usual/interfaces/ISmartMLike.sol` -- `src/usual/interfaces/IUsualM.sol` -- `src/usual/constants.sol` -- `src/usual/UsualM.sol` +- `m-portal/src/**` +- `common/src/**` +- `protocol/src/**` +- `ttg/src/**` --- # Findings Summary -| ID | Title | Severity | Status | -|--------|------------------------------------------------------------------|---------------|--------------| -| [H-01] | Incorrect decimals used for validating Chainlink oracle response | High | | -| [I-01] | Permit front-running causes revert in `wrapWithPermit` | Informational | Acknowledged | -| [I-02] | Missing support for full balance wrapping via `wrapWithPermit` | Informational | Acknowledged | -| [I-03] | Typos in NatSpec comments | Informational | | +| ID | Title | Severity | Status | +|--------|------------------------------------------------------------------------------------------------------------|---------------|--------| +| [M-01] | Accrual of unbacked yield when enabling earning for `HubPortal` | Medium | | +| [M-02] | Unchecked underflow in `outstandingPrincipal` calculation | Medium | | +| [L-01] | Missing `additionalPayload` length validation | Low | | +| [L-02] | Missing `TransferRedeemed` event emittance in `_receiveMToken` | Low | | +| [L-03] | Race condition in messages sent via `sendRegistrarListStatus` and `sendRegistrarKey` | Low | | +| [L-04] | Missing fee refund in `transferExcessM` | Low | | +| [I-01] | Incorrect NatSpec comment for `currentIndex` | Informational | | +| [I-02] | Unused function `toAddress` | Informational | | +| [I-03] | Use empty bytes for `DEFAULT_TRANSCEIVER_INSTRUCTIONS` | Informational | | +| [I-04] | Function `_receiveCustomPayload` in `SpokePortal` does not reject messages not coming from the `HubPortal` | Informational | | +| [I-05] | Typos in NatSpec comments | Informational | | +| [I-06] | Whitelisting smart contracts based on chain-agnostic addresses is error-prone | Informational | | # Security & Economic Findings -## [H-01] Incorrect decimals used for validating Chainlink oracle response +## [M-01] Accrual of unbacked yield when enabling earning for `HubPortal` -The default number of decimals for most Chainlink oracles is 8, which also applies to the newly deployed M NAV Chainlink -oracle available at `0xC28198Df9aee1c4990994B35ff51eFA4C769e534`. However, `NAVProxyMPriceFeed` incorrectly assumes the -Chainlink oracle has 6 decimals in `_getPriceFromNAV`. This may lead to incorrect values being reported by the proxy -oracle if the Chainlink NAV value drops below `1e8` (i.e., the proxy will keep reporting `1e6` instead of reporting -lower values). +Assuming `HubPortal` will be deployed and used before TTG governance approves its inclusion into the earners list, +earners that are already in the list can generate unbacked yield on M Token on L2. Consider the following series of +events: + +1. Portals are fully deployed and operational, however earning for `HubPortal` address hasn't been enabled yet (e.g. due + to TTG governance delay). +2. A user who is already an earner on L1 bridges their M token to L2 through `HubPortal` + 1. Index 0 is sent along with the M-minting message (since `_currentIndex()` returns 0) + 2. `SpokePortal` ignores received index 0, since it's lower than `_currentIndex()` initialized as `EXP_SCALED_ONE` + during M token deployment +3. User enables earning for their bridged M with contract assuming that the index is `EXP_SCALED_ONE` +4. TTG governance fully enables earning for `HubPortal` +5. Actual `index` is pushed to the L2 through the `HubPortal` +6. User M balance on L2 accrues yield assuming index changed from `EXP_SCALED_ONE` to the received `index`, while the + actual index increase on L1 has been much lower + +### Recommendation + +Disallow `startEarning()` on Spoke M Token if `currentIndex() == EXP_SCALED_ONE`. Rework `outstandingPrincipal` +accordingly. + +## [M-02] Unchecked underflow in `outstandingPrincipal` calculation + +Applied changes to `outstandingPrincipal` are always rounded down when calculating principal amount from present amount. +It's possible to cause unchecked underflow. For example, assume that index is 1.1 and 3 wei of M token is bridged in +twice, then 6 wei is bridged back (`floor(3 / 1.1) + floor(3 / 1.1) < floor(6 / 1.1)`). + +### Recommendation + +Adopt one of the following: + +* Round amounts that are bridged in upwards +* Remove `outstandingPrincipal` and count `excess` in Spoke M instead of `SpokePortal`, based on the + `totalNonEarningSupply` and change in the index as part of `updateIndex` + +## [L-01] Missing `additionalPayload` length validation + +The `PayloadEncoder` function `decodeTokenTransfer` does not validate length of the `additionalPayload`. Such a check is +present for all `payload` decoding functions, however. Since `additionalPayload` is also used to store `index` and has a +fixed length, its length validation is also expected. + +### Recommendation + +Add the following validation: + +```diff + TransceiverStructs.NativeTokenTransfer memory nativeTokenTransfer_ = TransceiverStructs + .parseNativeTokenTransfer(payload_); + + (index_, ) = nativeTokenTransfer_.additionalPayload.asUint64(0); ++nativeTokenTransfer_.additionalPayload.checkLength(8); +``` + +## [L-02] Missing `TransferRedeemed` event emittance in `_receiveMToken` + +The `Portal` contract overrides the `NTTManager` inbound message handling logic. The original logic contained a +`TransferRedeemed` event, which is not present in the overridden implementation. ### Recommendation -Update `NAVProxyMPriceFeed` decimals setting to 8 OR convert all values coming from the Chainlink's NAV oracle contract -from 8 decimals down to 6. +Add the missing emit statement in `receiveMToken`. + +## [L-03] Race condition in messages sent via `sendRegistrarListStatus` and `sendRegistrarKey` + +Wormhole does not provide strong guarantees about the delivery order of cross-chain messages. Therefore, applications +should assume that messages can be delivered in arbitrary order. The execution result of messages sent from `HubPortal` +to `SpokePortal` using `sendRegistrarListStatus` and `sendRegistrarKey` depends on their delivery order. + +For example, consider the following sequence of events: + +1. `sendRegistrarKey` for key `X` (message 1) +2. `setKey` for key `X` that updates the value +3. `sendRegistrarKey` for key `X` (message 2) +4. Message 2 gets delivered and executed +5. Message 1 gets delivered and executed + +As a result, values for key `X` will differ between Ethereum Mainnet and L2 until the key is sent again. The severity of +this issue is marked as Low, assuming all messages are relayed automatically via the Standard Wormhole Relayer. Note +that without an automated relayer, an attacker could pre-send many messages with old registrar values to stall key +update propagation for a prolonged time by reverting the key back to the original value immediately after someone tries +to update it using `sendRegistrarKey`. + +With all messages relayed automatically, the impact is limited to a small set of DOS opportunities. For example, an +attacker could break `enableEarning` in Smart M L2 deployment via the following sequence of events (e.g. 2-4 and 5-8 can +happen in the same block): + +1. TTG approves the L2 Smart M addition to the `EARNERS_LIST` +2. Attacker uses `sendRegistrarListStatus` to send `false` inclusion status for Smart M (message 1) +3. TTG proposal is executed, `addToList` is called +4. Attacker uses `sendRegistrarListStatus` to send `true` inclusion status for Smart M (message 2) +5. Message 2 gets delivered and executed (Smart M is added to the `EARNERS_LIST`) +6. Attacker calls `enableEarning()` on L2 Smart M +7. Message 1 gets delivered and executed (Smart M is removed from the `EARNERS_LIST`) +8. Attacker calls `disableEarning()` on L2 Smart M +9. It's no longer possible to re-enable earning via `enableEarning()` due to `EarningCannotBeReenabled()` revert + +### Recommendation + +Consider caching the last message sequence for each particular registrar key updated in `SpokePortal` and reverting all +messages that were sent before the last one applied to the particular key. + +## [L-04] Missing fee refund in `transferExcessM` + +The function `transferExcessM` calls `SpokePortal` to transfer M tokens back to Ethereum Mainnet. While doing so, it +forwards the `msg.value` to the `NTTManager`, which refunds the gas fee partially if the price quotes are updated (see +`_prepareForTransfer`). The gas fee is refunded to the `msg.sender`, which is the `SpokeVault` contract, however +`SpokeVault` does not forward it back to the caller. + +### Recommendation + +Add the following or similar refund forwarding logic to the `transferExcessM` function: + +```diff ++uint256 oldBalance_ = address(this).balance; + messageSequence_ = INttManager(spokePortal).transfer{ value: msg.value }( + amount_, + destinationChainId, + hubVault_, + refundAddress_, + false, + new bytes(1) + ); ++uint256 refund_ = address(this).balance - oldBalance_; ++if (refund_ > 0) { ++ (bool refundSuccessful,) = payable(msg.sender).call{value: refund_}(""); ++ ++ if (!refundSuccessful) { ++ revert RefundFailed(refund_); ++ } ++} +``` # Informational & Gas Optimizations -## [I-01] Permit front-running causes revert in `wrapWithPermit` +## [I-01] Incorrect NatSpec comment for `currentIndex` -The function `wrapWithPermit` reverts if the underlying call to `permit` fails. The call to `permit` can be front-run by -anyone once the transaction with `wrapWithPermit` is spotted in the mempool. Also, since `transferFrom` would revert -anyway if no sufficient allowance is present, `permit` call failures can be safely ignored to remove the risk of -transactions being reverted due to front-run. +The NatSpec comment for `currentIndex` in `IContinuousIndexing` references `updateIndex`, which no longer exists in the +spoke M version. Consider updating the NatSpec comment to reflect the current implementation. -Consider adopting a `tryPermit` pattern (e.g., similar to the one used -in [1inch protocols](https://github.com/1inch/solidity-utils/blob/12763d675b6318779a3e578b5ba1be65aff164bc/contracts/libraries/SafeERC20.sol#L300)). +```solidity +/// @notice The current index that would be written to storage if `updateIndex` is called. +function currentIndex() external view returns (uint128); +``` + +## [I-02] Unused function `toAddress` + +The `toAddress` function in the spoke version of `Registrar` is unused and can be safely removed. Consider removing it. + +## [I-03] Use empty bytes for `DEFAULT_TRANSCEIVER_INSTRUCTIONS` + +The `WormholeTransceiver` accepts empty transceiver instructions and defaults to `shouldSkipRelayerSend = false`, as +shown in `parseWormholeTransceiverInstruction`. To save gas and reduce calldata size, consider using empty bytes value +for `DEFAULT_TRANSCEIVER_INSTRUCTIONS` in both `HubPortal` and `SpokeVault`. + +## [I-04] Function `_receiveCustomPayload` in `SpokePortal` does not reject messages not coming from the `HubPortal` -## [I-02] Missing support for full balance wrapping via `wrapWithPermit` +The `_receiveCustomPayload` function in `SpokePortal` is intended to only handle messages from `HubPortal`. However, it +does not validate the source chain ID of delivered messages. The underlying `NttManager` treats all messages from +whitelisted peers the same way. As an extra safeguard, consider validating source chain ID according to the "Principal +Of Least Authority". -There is useful `wrap` functionality in `UsualM` that wraps all available balance, which greatly improves the UX for -rebasing tokens. However, no corresponding `wrapWithPermit` alternative exists. +## [I-05] Typos in NatSpec comments -Consider adding a similar `wrapWithPermit` overload that uses a `permit` signature to obtain an infinite allowance but -transfers only the available M Token balance. +In the `ISpokeVault.sol`: -## [I-03] Typos in NatSpec comments +```diff +-* @param refundAddress The address to which a refund for unussed gas is issued on the destination chain. ++* @param refundAddress The address to which a refund for unused gas is issued on the destination chain. +``` -In the `IUsualM.sol`: +In the `SpokePortal.sol` ```diff --/// @notice Returns wheather account is blacklisted. -+/// @notice Returns whether account is blacklisted. +-/// @dev Decreases `outstandingPrincipal` after M tokens are transfered out, ++/// @dev Decreases `outstandingPrincipal` after M tokens are transferred out, ``` + +## [I-06] Whitelisting smart contracts based on chain-agnostic addresses is error-prone + +The current implementation of the `EARNERS` Registrar whitelist is chain-agnostic, meaning that the same address is +treated identically across different L2s. This implicitly assumes that the same address on different chains is +controlled by the same party. However, for smart contracts, this assumption may not hold true. For example, if the +deployer key for `HubPortal` is compromised, it could be used to deploy malicious contracts on L2s with the same address +to gain unauthorized access to earning functionality. Since `HubPortal` is a system contract designed to always be +earning, there would be no straightforward way to block the malicious party's earning access. + +Consider either: + +1. Maintaining separate earner whitelists for different chains, or +2. Modifying the deployment process to use deterministic `CREATE2` deployments for contracts like `HubPortal` and + `SmartMToken` (e.g., Foundry scripts use https://github.com/Arachnid/deterministic-deployment-proxy by default for + `CREATE2` calls).