Unchecked return value for `emergencyExecute()` in the `PartyGovernance` contract

[code423n4]

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol#L795

Vulnerability details

Description

The PartyGovernance.sol contract implements an emergencyExecute() function which allows the DAO to execute arbitrary functions in the event of an emergency provided onlyWhenEmergencyExecuteAllowed is set to true. There is a defined bool which stores the successful or failure execution of the low level call() function against the targetAddress. This value is not checked regardless of if the call was successful or not.

This was awarded a "Medium" in severity because there wouldn't be an immediate way of knowing if the call was successful or not.

Impact

Whilst the emergency action(s) must not be revoked for the call to emergencyExecute(), in the event of a failure the result of the call will go unnoticed if it isn't checked.

Proof of Concept

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol#L795

Tools Used

Manual code inspection

Recommended Mitigation Steps

Consider applying a require() statement to check to see if the call was successful or not. If a require() statement is not desirable, at least consider emitting an event the result of the call to allow for off chain monitoring to immediately respond to the failure.

[merklejerk]

Will add a revert if the call fails.

[0xble]

Resolved: https://github.com/PartyDAO/partybidV2/pull/125

[HardlyDifficult]

This is a fair recommendation. Downgrading since this is an admin function. It returns the value so they could check via a static call but of course there could be other transactions in the block making this not 100% reliable. They could also trace a tx afterwards. Merging with #250