Moonwell - Vagner's results

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L344-L409

Vulnerability details

Impact

The function executeProposal is used with the wormhole bridge to execute proposals from different chains, but it doesn't check if the VAA processed is meant to be sent to that address.

Proof of Concept

As you can see it is specified in both queueProposal and executeProposal that is important that the VAA is processed only once and critically, intended for that contract https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L226 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L235 For queueProposal case this is correctly checked in this require statement https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L322 but there is not checks in the executeProposal proposals, which could lead to wrong assumptions, doing external calls with different values to random addresses in the case where the intended address is not TemporalGovernor.sol. This is especially dangerous fastTrackProposalExecution where checks like queueTime would pass, and the calls will be done immediately. This could also lead to loss of funds is value is meant to be sent with those calls.

Tools Used

Manual review

Recommended Mitigation Steps

Do the same checks in _executeProposal like in _queueProposal since you specify in the comments that it is critically to check this.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L400-L402

Vulnerability details

Impact

The contract TemporalGovernor.sol is used to queue and execute proposals on other chains with data and values, but that is not possible since the contract can't receive native tokens.

Proof of Concept

As you can see the function executeProposal and fastTrackProposalExecution are not payable https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L237 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L266 and the contract doesn't have any receive function, nor the contracts that it inherits, so any time the _executeProposal would be called with any of the values array value greater than 0, the transaction would revert because there are no native funds in the contract to be transferred.

Tools Used

Manual review

Recommended Mitigation Steps

Implement a receive function or set any function in the contract payable to be able to receive native tokens.

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L1031-L1052

Vulnerability details

Impact

The way the rewards are distributed right now, in the MultiRewardDistributor.sol, can make the whole distribute process unusable because of out-of-gas errors.

Proof of Concept

The way the whole claiming rewards process, does multiple interactions with multiple contracts and updates multiple storage values in multiple loops, which could lead easily to out-of-gas errors, but let's see the whole claiming process.

• the claiming start from the Comptroller.sol contract where a user can claim rewards for all/multiple MTokens, or just one if the array provided contains one, we will take the only one example since that will cost the lowest amount of gas.
• claimReward function will be called by an user with only his address and one MToken, taking both rewards for borrowers and suppliers
• the function will do an external call to rewardDistributor which is the MultiRewardDistributor.sol which already cost more gas then calling the functions internally in the MultiRewardDistributor.sol
• first updateMarketSupplyIndex is called https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L1039 which will call updateMarketSupplyIndexInternal in MultiRewardDistributor.sol, function which loops over some storage variables, and also updates some of those storage variables as can be seen here https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1009-L1024, this means that every SLOAD on a slot that wasn't accessed yet, which will be the case here, will be around 2100 gas, and every SLOAD done in this function that wasn't access yet would cost around 5000, since the values would be updated from non-zero to non-zero value, which happens 2 times in every loop https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1022-L1024
• after that disburseSupplierRewards is then called on the MultiRewardDistributor.sol, another external call, which calls disburseSupplierRewardsInternal, function which again loops trough multiple storage values and updates some https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1053-L1068, this time though the SLOAD for accessed slots would cost just 100 gas, only the new slots accessed would cost 2100, but the SSTORE in this function would cost 22100 for the first time an user call this function for that specific marketConfig, since it updates the value from zero to non-zero on a slot that wasn't accessed yet, which happens 2 times, for every loop, as an example for 3 loops it would cost around 132,600 just to update those 2 variables.
• after that it will send the reward tokens on every marketConfig which again can cost a lot of gas, depending on what logic there is on the specific token contract (if it checks for blacklist for exemple)
• then it will do the same thing for the borrowers part by calling updateMarketBorrowIndex and disburseBorrowerRewards which does exactly the same things as I specified above, but on new slots, which again costs a lot of gas. All of these calculations can very easily get to too much gas consumed by this claiming process, which will make the claims impossible, especially because the whole marketConfigs can't be removed, as it is stated in the NatSpec https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L35-L36 which means that every user would have to loop trough every config ever added with _addEmissionConfig doing the whole process that I talked above. Keep in mind that I talked only for claiming rewards, for only one MToken on only one user, for more than that it will get way more costly which will mostly fail for sure.

Tools Used

Manual review

Recommended Mitigation Steps

Consider splitting the functionality a bit, the claimReward function on Comptroller.sol does too many things at once, also try to work more with memory values by coping the storage values into memory, and then loop trough them, as it cost way less gas, and only update the storage values at the end

Assessed type

Other