code-423n4 / 2023-07-moonwell-findings

1 stars 0 forks source link

`executeProposal` in `TemporalGovernor.sol` doesn't check if the VAA is intended for the contract called as it should #280

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

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

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #308

c4-judge commented 1 year ago

alcueca marked the issue as satisfactory

c4-judge commented 1 year ago

alcueca marked the issue as partial-50

c4-judge commented 1 year ago

alcueca changed the severity to 2 (Med Risk)