Moonwell - Aymen0909's results

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L261-L268

Vulnerability details

Impact

The function fastTrackProposalExecution is supposed to be used by the TemporalGovernor owner during periods of emergency, when the contract is paused to fast track a certain proposal (queue and execute a proposal directly without delay or voting), with gaol of ensuring the protocol safety.

But the function fastTrackProposalExecution is missing the whenPaused modifier which means it can be called at any time to fast track any proposal and not just when the TemporalGovernor is paused.

I submit this issue as only Medium risk because only the TemporalGovernor owner can call the fastTrackProposalExecution function (and i suppose that the owner is trusted and will not execute malicious actions), but this issue still goes against the intent of the governance process in which the members of the DAO are supposed to propose, vote and execute the proposals in a permissionless and decentralized manner and where the owner should not be able to execute actions on the protocol without the agreement of the members (except for those emergency situation of course).

Proof of Concept

The issue is present in the fastTrackProposalExecution function below :

/// @notice Allow the guardian to process a VAA when the
/// Temporal Governor is paused this is only for use during
/// periods of emergency when the governance on moonbeam is
/// compromised and we need to stop additional proposals from going through.
/// @param VAA The signed Verified Action Approval to process
function fastTrackProposalExecution(bytes memory VAA) external onlyOwner {
    // @audit can be called when contract is not paused 
    _executeProposal(VAA, true); /// override timestamp checks and execute
}

As it can be seen the function does not contain the whenPaused modifier and thus it can be called at any moment by the owner to fast track a proposal.

We can also notice that the function comments do mention the fact that it should only be called when the Temporal Governor is paused.

Tools Used

Manual review

Recommended Mitigation Steps

I recommend to add the whenPaused modifier to the fastTrackProposalExecution function, in order to ensure that the governance process will always work as a real DAO and the owner only intervene in the emergency cases.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L564-L571 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L605-L612

Vulnerability details

Impact

In the MultiRewardDistributor contract, the _addEmissionConfig function allow the admin (timelock) to add multiple emission config to a given MToken, each emission config corresponds to a certain emission token (ERC20).

All the emission configs are stored in a list marketConfigs[mtoken], and there is no limit on the number of emission config that can be added to this list, so the admin (timelock) is allowed to add multiple emission config.

This can cause an issue in some protocol functions that loops over all those emission configs in their logic, as if the number of emission configs stored in the list marketConfigs[mtoken] get relatively big, those function can run out of gas which will make them impossible to call (cause a DOS of their functionality).

Some of the affected functions could be : updateMarketSupplyIndexAndDisburseSupplierRewards, updateMarketBorrowIndexAndDisburseBorrowerRewards.

Proof of Concept

This issue can occur if the number of emission configs for a given MToken become relatively big, the _addEmissionConfig function allow the admin (timelock) to add multiple configs (if the emission token remain unique) for each MToken.

The risk of this issue causing a run out of gas denial of service is very low for the functions that only loops once over all the emission configs like the functions : updateMarketSupplyIndex, disburseSupplierRewards, updateMarketBorrowIndex, disburseBorrowerRewards. But the risk of DOS gets higher when a function run the loop twice (over all the emission configs) as it is the case in the functions : updateMarketSupplyIndexAndDisburseSupplierRewards, updateMarketBorrowIndexAndDisburseBorrowerRewards.

If we take for example the function updateMarketSupplyIndexAndDisburseSupplierRewards :

function updateMarketSupplyIndexAndDisburseSupplierRewards(
    MToken _mToken,
    address _supplier,
    bool _sendTokens
) external onlyComptrollerOrAdmin {
    updateMarketSupplyIndexInternal(_mToken);
    disburseSupplierRewardsInternal(_mToken, _supplier, _sendTokens);
}

It call both functions updateMarketSupplyIndex and disburseSupplierRewards and thus perform two loops over the emission config list.

The updateMarketSupplyIndexAndDisburseSupplierRewards is used in the updateAndDistributeSupplierRewardsForToken function inside the Comptroller contract, the later function is implemented in many critical functions : a single time in both mintAllowed, redeemAllowed and twice in seizeAllowed, transferAllowed.

So in the case the number of emission configs becomes relatively big (not necessarily too big), the function updateMarketSupplyIndexAndDisburseSupplierRewards will be blocked which could block those functions especially the two functions seizeAllowed, transferAllowed because they basically loop 4 time over the configs list (which will cost a lot of gas).

The same issue could happen with the updateAndDistributeBorrowerRewardsForToken function which is implemented in borrowAllowed, repayBorrowAllowed.

If those functions are blocked, it will negatively impact the protocol as no mint, borrow, repay, redeem or transfer operation will be allowed which are among the main functionality of the protocol.

This issue is even more critical as there is no way to remove certain emission configs that were added to marketConfigs[mtoken] list.

Tools Used

Manual review

Recommended Mitigation Steps

I recommend to add a maximum limit on the number of emission configs associated to each MToken.

Assessed type

DoS