Moonwell - 0x70C9's results

Lines of code

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

Vulnerability details

Impact

The TemporalGovernor contract has no payable methods, not even a payable receive / fallback function. The only way to put money in it is through selfdestruct (deprecated) or validator reward transfer, which is the same as saying that it was not designed to receive native token.

However, there's nothing stopping proposals to be queued and executed expecting calls to targets with native token value in them. We can see this was expected in _executeProposal's actual execution logic:

address target = targets[i];
uint256 value = values[i];
bytes memory data = calldatas[i];

// Go make our call, and if it is not successful revert with the error bubbling up
(bool success, bytes memory returnData) = target.call{value: value}(
  data
);
// @audit-issue any non zero value will make this call unsuccessful.

Proof of Concept

All we need is a non-zero value proposal coming from Wormhole, this contract is not expected to have value neither has a good way of providing it, so critical governance proposals might never be able to get executed in the chain where this contract is deployed.

Tools Used

Manual review.

Recommended Mitigation Steps

Either add some payable receive method or explicitly revert queued proposals with non-zero value.

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L146-L156 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L173-L183

Vulnerability details

Impact

The trustedSenders array makes use of EnumerableSet. When doing an addition of an already present element, the EnumerableSet add method will return false. However, function setTrustedSenders does not check this return value and it still emits a TrustedSenderUpdated event, even though no trusted sender got changed.

A similar thing can be said regarding function unSetTrustedSenders and EnumerableSet's remove method: when the element is not present in the array in the first place, an event still gets emitted.

Tools Used

Manual review.

Recommended Mitigation Steps

Check the EnumerableSet's return value to know whether or not it should emit an event.

Assessed type

Loop