no control to to `cancelTransaction()` by `veto()` and `cancel()`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L351 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L376 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOExecutor.sol#L134

Vulnerability details

Impact

Losing the access to cancelTransaction() by veto() and cancel()

Proof of Concept

On veto() we see require(msg.sender == vetoer, '...'); and on cancel() require(msg.sender == proposal.proposer || …'...' ); These function invoking cancelTransaction() which has a require(msg.sender == admin, '...'); so no one of them can enter cancelTransaction() unless cancel() when passed by the second part of the require

Recommended Mitigation Steps

Edit the check patterns

[davidbrai]

cancelTransaction makes sure that the DAOLogic is the one that called it. msg.sender changes when calling an external contract.