Cap Manual Review Findings

CPA-01M: Inexistent Initialization Protection of Base Implementation

Type	Severity	Location
Language Specific		Cap.sol:L44

Description:

The contract is meant to be upgradeable yet does not properly protect its logic deployment from malicious initializations.

Example:

Copy
36contract Cap is ICap, OwnableUpgradeable, Router, CapStorageV1 {
37    using EnumerableMapExtended for EnumerableMapExtended.UintToBytes32Map;
38
39    event AddLoad(address indexed vault, address indexed spender, uint256 indexed epoch, uint256 amount);
40    event SubLoad(address indexed vault, address indexed saver, uint256 indexed epoch, uint256 amount);
41    event AcceptRemoteEpochDone(uint32 indexed domain, uint256 indexed epoch);
42    event MoveToNextEpoch(uint256 indexed epoch, uint256 prevCap, uint256 newCap);
43
44    constructor(address mailbox) Router(mailbox) {}
45
46    function initialize(address owner, address hook, address ism) public initializer {
47        _MailboxClient_initialize(hook, ism, owner);
48
49        StorageV1 storage $ = _getStorageV1();
50        $._epoch[localDomain] = 0;
51        $._load = 0;
52        $._ready = false;
53    }

Recommendation:

We advise a constructor to be introduced that either invokes the initializer modifier of the Initializable contract or invokes the Initializable::_disableInitializers function to prevent the base implementation from ever being initialized.

Alleviation (58e8cc66dfa900c03c47df78f5170d9960005629):

An Initializable::initialize modifier has been introduced to the contract's Cap::constructor thereby preventing re-initializations as long as the contract does not utilize a versioned initialization system.

If such a system is expected, we advise the Initializable::_disableInitializers function instead.

CPA-02M: Inexistent Validation of Ascending Cap Order

Type	Severity	Location
Input Sanitization		Cap.sol:L168-L171

Description:

Based on the overall Mitosis system, each _cap that is configured for the next epoch should be greater than the last; a trait which is not enforced in the codebase.

Impact:

A severity of minor has been assigned due to the function's administrative nature.

Example:

Copy
168/// @dev pre-set cap for the epoch
169function setEpochCap(uint256 epoch_, uint256 cap_) external onlyOwner {
170    _getStorageV1()._cap[epoch_] = cap_;
171}

Recommendation:

We advise the Cap::setEpochCap function to ensure that the cap_ being set is greater than the previous one and less than the next one if the next one exists, ensuring that the caps are ordered in a strictly ascending fashion within the _cap data entry.

Alleviation (5297bb74fa):

The Cap::setEpochCap function was significantly refactored to ensure the ascending cap order, however, this is done very inefficiently.

We advise validation to only be performed between the inserted cap and the one before and after it, as this would guarantee all caps remain ordered per adjustment in a much more efficient manner.

Alleviation (58e8cc66df):

The Mitosis team opted to retain the last implementation in place, iterating through all caps and ensuring their order.

Given that the original vulnerability has been rectified, we consider this exhibit adequately albeit inefficiently alleviated.

CPA-03M: Inexistent Validation of Epoch Configuration

Type	Severity	Location
Input Sanitization		Cap.sol:L173-L190

Description:

The Cap::sudoEpoch will not validate that a _cap has been set for the epoch_ being configured which contradicts how the Cap::_checkRemoteStateAndAdvance function operates.

Impact:

A severity of minor has been assigned due to the administrative nature of the Cap::sudoEpoch function.

Example:

Copy
173/// @dev force set epoch & broadcast to all routers
174/// @notice this is a sudo function and should be used with caution
175function sudoEpoch(uint256 epoch_) external onlyOwner {
176    StorageV1 storage $ = _getStorageV1();
177
178    $._ready = false;
179    if ($._epoch[localDomain] < epoch_) {
180        revert Error.InvalidEpoch("epoch should be greater than current epoch");
181    }
182    $._epoch[localDomain] = epoch_;
183
184    uint32[] memory domains = _routers.uint32Keys();
185    bytes memory msgCapFilled = abi.encode(epoch_);
186
187    for (uint256 i = 0; i < domains.length; i++) {
188        _dispatch(domains[i], msgCapFilled);
189    }
190}

Recommendation:

We advise the same validation to be imposed despite the function's administrative nature to ensure that the system cannot be forced to an invalid state.

Alleviation (5297bb74fa5cb1c63239172a7a7a3a7c8ce808e3):

An if-revert pattern check was introduced ensuring that a _cap has been properly specified for the forced epoch_, alleviating this exhibit.

CPA-04M: Trivial Epoch Advancement

Type	Severity	Location
Logical Fault		Cap.sol:L133-L151, L153-L166

Description:

The Cap::addLoad function will automatically advance the epoch if the cap has been reached via the Cap::_processDone function, however, a withdrawal that will reduce the _load below the cap will not reset this request.

As such, it is trivial to force epochs to advance by depositing, dispatching a message to advance, and withdrawing the funds that were necessary to reach the cap at no cost.

Impact:

A user can trivially force the system to increase its cap while the actual _load of the system does not correlate to these cap increases.

Example:

Copy
133/// @return added actual amount
134/// @dev [IMPORTANT] probably amount needs to be normalized
135/// @dev if load + amount > cap, then only spend remaining cap
136/// @dev if cap and load is same, then revert with InsufficientCap
137function addLoad(uint256 amount, address spender) external onlyManager returns (uint256) {
138    uint256 added = calcAdded(amount);
139
140    StorageV1 storage $ = _getStorageV1();
141
142    $._load += added;
143
144    if (cap(_epoch()) == $._load) {
145        _processDone();
146    }
147
148    emit AddLoad(_msgSender(), spender, _epoch(), added);
149
150    return added;
151}
152
153/// @return subbed actual amount
154/// @dev if load is 0, then revert with InsufficientLoad
155/// @dev if load < amount, then only save remaining load
156function subLoad(uint256 amount, address saver) external onlyManager returns (uint256) {
157    uint256 subbed = calcSubbed(amount);
158
159    StorageV1 storage $ = _getStorageV1();
160
161    $._load -= subbed;
162
163    emit SubLoad(_msgSender(), saver, _epoch(), subbed);
164
165    return subbed;
166}

Recommendation:

We advise this approach to be revised, potentially prohibiting withdrawals while an epoch advancement is in progress.

Alleviation (58e8cc66df):

The Mitosis team has opted to acknowledge this exhibit, permitting advancements to occur trivially.

As this exhibit remains open, the audit report cannot be publicly released as it would effectively instruct users how to sabotage the system.

Alleviation (67e2cd4ae4):

The Mitosis team imposed a hard limitation on the number of external domains a synchronization will happen, constraining the upper limit gas cost of each epoch advancement.

The Mitosis team considers the gas concerns raised by this exhibit as safe to acknowledge in light of this change, and as such we consider this exhibit as acknowledged.

CPA-05M: Incorrect Initialization of Minimum-Value Search

Type	Severity	Location
Logical Fault		Cap.sol:L259

Description:

The Cap::_checkRemoteStateAndAdvance function will incorrectly initialize the nextEpoch as domains[0] instead of $._epoch[domains[0]], causing the inner for loop to ignore the first domain minimum epoch.

Impact:

The Cap system may presently advance even if the first epoch is not in sync with the rest of the system.

Example:

Copy
253/// @dev if there's no epoch cap info to advance, we just keep the current epoch. - nothing will happen
254/// @dev "domains" does not includes current local domain
255function _checkRemoteStateAndAdvance(uint32[] memory domains) internal {
256    StorageV1 storage $ = _getStorageV1();
257
258    uint256 currentEpoch = $._epoch[localDomain];
259    uint256 nextEpoch = domains[0];
260
261    // 1. find the minimum epoch among all remote chains.
262    // 2. check the every remote chain's epoch is same as the min epoch.
263    for (uint256 i = 1; i < domains.length; i++) {
264        uint256 epochForDomain = $._epoch[domains[i]];
265
266        if (epochForDomain < nextEpoch) {
267            nextEpoch = epochForDomain;
268        }
269        if (epochForDomain != nextEpoch) {
270            return;
271        }
272    }
273
274    if (currentEpoch < nextEpoch) {
275        if ($._cap[nextEpoch] == 0) {
276            return;
277        }
278
279        $._ready = false;
280        $._epoch[localDomain] = nextEpoch;
281
282        emit MoveToNextEpoch(nextEpoch, $._cap[currentEpoch], $._cap[nextEpoch]);
283    }
284}

Recommendation:

We advise the nextEpoch value to be correctly initialized, ensuring that the Cap::_checkRemoteStateAndAdvance function will correctly identify the minimum epoch across domains.

Alleviation (5297bb74fa5cb1c63239172a7a7a3a7c8ce808e3):

The nextEpoch value is now correctly initialized, ensuring that the minimum epoch present is correctly identified.