Open code423n4 opened 12 months ago
This looks like the intended mechanism to avoid malicious proposal being executed originated from the malicious guardians.
0xSorryNotSorry marked the issue as primary issue
good finding and this is expected behavior as wormhole guardians may change if signers are ever compromised
ElliotFriedman marked the issue as sponsor confirmed
alcueca marked the issue as satisfactory
alcueca marked the issue as selected for report
Lines of code
https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L346-L350
Vulnerability details
Impact
Wormhole governance can change signing guardian sets. If this happens between a proposal is queued and a proposal is executed. The second verification in
_executeProposal
will fail as the guardian set has changed between queuing and executing.This would cause a proposal to fail to execute after the
proposalDelay
has passed. Causing a lengthy re-sending of the message fromTimelock
on source chain, thenproposalDelay
again.Proof of Concept
To execute a proposal on
TemporalGovernor
it first needs to be queued. When queued it is checked that the message is valid against the Wormhole bridge contract:https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L295-L306
After some more checks are done the message is queued by its wormhole hash.
Then, after
proposalDelay
, anyone can callexecuteProposal
Before the proposal is executed though, a second check against the Wormhole bridge contract is done:
https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L344-L350
The issue is that the second time the verification might fail.
During the wormhole contract check for validity there is a check that the correct guardian set has signed the message:
https://github.com/wormhole-foundation/wormhole/blob/f11299b888892d5b4abaa624737d0e5117d1bbb2/ethereum/contracts/Messages.sol#L79-L82
But(!), Wormhole governance can change the signers:
https://github.com/wormhole-foundation/wormhole/blob/f11299b888892d5b4abaa624737d0e5117d1bbb2/ethereum/contracts/Governance.sol#L76-L112
Were this to happen between
queueProposal
andexecuteProposal
the proposal would fail to execute.Tools Used
Manual audit
Recommended Mitigation Steps
Consider only check the
VAA
s validity against Wormhole in_executeProposal
when it is fasttracked (overrideDelay==true
).However then anyone can just just take the hash from the proposal
VAA
and submit whatever commands. One idea to prevent this is to instead of storing theVAA
vm.hash
, use the hash of the completeVAA
as key inqueuedTransactions
. Thus, the content cannot be altered.Or, the whole
VAA
can be stored alongside the timestamp and theexecuteProposal
call can be made with just the hash.Assessed type
Timing