Omniscia Mitosis Audit

BasicVault Manual Review Findings

BasicVault Manual Review Findings

BVT-01M: Bypass of Halt Restrictions

Description:

The BasicVault::manualDeposit and BasicVault::manualRedeem functions will bypass the BasicVault::withNoHalt security modifier.

Impact:

The severity of this exhibit will be adjusted based on the actions of the Mitosis team.

Example:

src/vault/BasicVault.sol
168function manualDeposit(uint256 amount, address receiver) external returns (uint256) {
169    if (!_isAllowed(_msgSender(), Action.Mint)) {
170        revert Error.Unauthorized();
171    }
172

173    uint256 added = _getUtilStorageV1()._cap.addLoad(amount, receiver);
174

175    _deposit(added, receiver);
176

177    return added;
178}
179

180function manualRedeem(uint256 amount, address receiver) external returns (uint256) {
181    if (!_isAllowed(_msgSender(), Action.Burn)) {
182        revert Error.Unauthorized();
183    }
184

185    uint256 subbed = _getUtilStorageV1()._cap.subLoad(amount, receiver);
186

187    _redeem(subbed, receiver);
188

189    return subbed;
190}

Recommendation:

We advise the Mitosis team to evaluate whether this is intended as the BasicVault::withNoHalt function failing indicates a critical failure in the contract that should deny cross-chain messages.

Alleviation (d94d2a63d25db5623d69dc33aea6e4fdd011d669):

The BasicVault::withNoHalt modifier has been properly introduced to the referenced functions, alleviating this exhibit.

BVT-02M: Improper Deposit Log Maintenance

Description:

The first DepositLog introduced to the logs of the receiver will have a cumulative value of 0 whilst it should be initialized to the amount deposited.

Impact:

This recommendation is simply semantic as the cumulative value inclusive of the latest deposit can be calculated by summing cumulative with amount. As such, we will assign a severity depending on the intended approach by the Mitosis team.

Example:

src/vault/BasicVault.sol
242DepositLog memory log = DepositLog({cumulative: 0, amount: amount, at: block.timestamp});
243

244if (info.logs.length > 0) {
245    log.cumulative = info.logs[info.logs.length - 1].cumulative + amount;
246}

Recommendation:

We advise the cumulative value to be set to amount as well, ensuring that it properly represents the cumulative deposits up to and including the latest deposit.

Alleviation (5297bb74fa5cb1c63239172a7a7a3a7c8ce808e3):

The Mitosis team evaluated this exhibit and opted to retain the current behaviour in place as the semantic they wish to follow is to explicitly sum the cumulative and amount values of a log.

As such, we consider this exhibit nullified as it described desirable behaviour.

BVT-03M: Inexistent Initialization Protection of Base Implementation

Description:

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

Example:

src/vault/BasicVault.sol
71contract BasicVault is
72    IVault,
73    ERC20PermitUpgradeable,
74    OwnableUpgradeable,
75    BasicVaultStorageV1,
76    BasicVaultUtilStorageV1
77{
78    event Deposit(address indexed account, address indexed token, uint256 amount);
79    event Redeem(address indexed account, address indexed token, uint256 amount);
80

81    event Halted(Action action);
82    event Resumed(Action action);
83

84    function initialize(address owner, IERC20 asset, string memory name, string memory symbol) public initializer {
85        __ERC20_init(name, symbol);
86        __ERC20Permit_init(name);
87        __Ownable_init();
88        _transferOwnership(owner);
89

90        StorageV1 storage $ = _getStorageV1();
91        $._underlyingDecimals = IERC20Metadata(address(asset)).decimals();
92        $._asset = asset;
93    }

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):

A BasicVault::constructor has been introduced invoking the Initializable::initialize modifier 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.

BVT-04M: Inexistent Consumption of Allowance

Description:

As evidenced in EETHDepositHelper::_redeem, the BasicVault::_redeem function should consume allowance from the caller to burn their tokens but it does not.

Impact:

Redemption operations on the BasicVault do not require an allowance which goes against common practice.

Example:

src/vault/BasicVault.sol
228function _redeem(uint256 amount, address receiver) internal {
229    _pruneDepositLog(amount, receiver);
230    _burn(_msgSender(), amount);
231    emit Redeem(receiver, address(_getStorageV1()._asset), amount);
232}

Recommendation:

We advise allowance to be incorporated, ensuring that the caller has correctly authorized the redemption operation to the BasicVault.

Alleviation (d94d2a63d2):

The Mitosis team evaluated this exhibit and opted to retain the current behaviour in place as they deem explicit approvals a waste of gas.

We would like to note that in such a case, the approval operation of the EETHDepositHelper::_redeem function needs to be removed rendering this exhibit partially alleviated.

Alleviation (58e8cc66df):

The approval increase in EETHDepositHelper::_redeem has been removed as advised, fully alleviating this exhibit.

BVT-05M: Improper Assumption of Binary Search

Description:

The BasicVault::_pruneDepositLog function assumes that the binary search it implements will always detect a deposit log whose redemption period has elapsed; this is not always the case as the binary search will terminate with a valid index but potentially invalid log.

Impact:

The redemption period is presently not enforced, effectively representing as an absent feature of the system.

Example:

src/vault/BasicVault.sol
251function _pruneDepositLog(uint256 amount, address receiver) internal {
252    UtilStorageV1 storage $ = _getUtilStorageV1();
253

254    if ($._redeemPeriod <= 0) {
255        return;
256    }
257

258    DepositInfo storage info = $._deposits[receiver];
259

260    if (info.logs.length == 0) {
261        info.resolved -= amount;
262        return;
263    }
264

265    uint256 high = info.logs.length;
266    uint256 low = info.lastLogIdx;
267

268    while (low < high) {
269        uint256 mid = Math.average(low, high);
270        if (info.logs[mid].at + $._redeemPeriod > block.timestamp) {
271            high = mid;
272        } else {
273            low = mid + 1;
274        }
275    }
276

277    uint256 extracted = info.logs[high - 1].cumulative - info.logs[info.lastLogIdx].cumulative;
278

279    info.lastLogIdx = high - 1;
280    info.resolved = (info.resolved + extracted) - amount;
281

282    return;
283}

Recommendation:

We advise the code to introduce one more conditional that ensures the info.logs entry that the binary search yielded has had its redemption period elapsed, ensuring that only redeemable logs can be consumed by redemption operations on the BasicVault.

Alleviation (58e8cc66df):

The code has been refactored across multiple functions to properly support the redeemPeriod mechanism of the contract, yielding a bool that indicates a claimable entry has been found in the BasicVault::_searchClaimableDepositIndex function while conditionally pruning and ensuring the redemption amount has been met in the BasicVault::_redeem function.

As a result of these changes, we consider the redemption period limitation properly upheld by the contract in all circumstances apart from transfers. Specifically, a user can circumvent the redemption period limitation by transferring their EIP-20 balance to a secondary account and redeeming from it.

We advise the code to permit transfers of up to the redeemable amount instead, properly maintaining the deposit logs as if a "withdrawal" occurred for the redeemed assets. Identically, the balance of a user should be split between assets-to-be-unlocked via the redemption period and already-unlocked balance that they can freely transfer.

Alleviation (67e2cd4ae4):

The deposit log and redemption period concepts have been eliminated from the codebase entirely, rendering this exhibit no longer applicable.

BVT-06M: Improper Protection Against EIP-777 Re-Entrancies

Description:

The BasicVault::deposit function, despite what the in-line documentation states, will result in a corrupted system state during an EIP-777 re-entrancy. Specifically, the Cap::addLoad function will be invoked before the transfer is performed which will update the _load of the particular epoch as well as relay messages if need be to other chains.

This can cause redemptions of higher amounts than permitted to occur before the actual deposit goes through which is ill-advised.

Impact:

The system is presently insecure against EIP-777 re-entrancies as it will contain a corrupt system state during any re-entrancy be it local or cross-contract.

Example:

src/vault/BasicVault.sol
135function deposit(uint256 amount, address receiver) external withNoHalt(Action.Deposit) {
136    StorageV1 storage $ = _getStorageV1();
137

138    uint256 added = _getUtilStorageV1()._cap.addLoad(amount, receiver);
139

140    // If _asset is ERC777, `transferFrom` can trigger a reentrancy BEFORE the transfer happens through the
141    // `tokensToSend` hook. On the other hand, the `tokenReceived` hook, that is triggered after the transfer,
142    // calls the vault, which is assumed not malicious.
143    //
144    // Conclusion: we need to do the transfer before we mint so that any reentrancy would happen before the
145    // assets are transferred and before the shares are minted, which is a valid state.
146    // slither-disable-next-line reentrancy-no-eth
147    SafeERC20.safeTransferFrom($._asset, _msgSender(), address(this), added);
148

149    _deposit(added, receiver);
150}

Recommendation:

We recommend that the statements are re-ordered, performing the inbound token transfer at the very beginning of the function and thus ensuring that the system contains a valid state at all times.

Alleviation (5297bb74fa5cb1c63239172a7a7a3a7c8ce808e3):

The transfer of the EIP-20 / EIP-777 asset has been re-ordered to be performed before the Cap::addLoad function invocation, alleviating this exhibit.

Prev

ArbitrumBridgeAdapter.sol (ABA-M)

Next

BasicVaultFactory.sol (BVF-M)

On this page