Incorrect Withdraw Pattern

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/c1c7c6201d0247f92472419ff657b570f9104565/contracts/governance/NounsDAOLogicV2.sol#L783-L792

Vulnerability details

Context:
NounsDAOLogicV2.sol#L783-L792

Description:

1 -When we transfer ether with call, we have to check (with require) whether the bool value will be successful.This part is missing in the code in the contract
Proof Of Concept: https://solidity-by-example.org/sending-ether/

2- Since the bool value is not checked with require, the value value in "emit" will be recorded with an incorrect value, as if the transfer was successful, even if there is no ether transfer.

3- call in combination with re-entrancy guard is the recommended method to use after December 2019.
Proof Of Concept: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

4- This can be unsafe if the private key for the owner account falls into the wrong hands, allowing instant withdrawal of all the funds. In general, having a single point of failure like this is not recommended best practice .See this example where a similar finding has been flagged as a high-severity issue
Proof Of Concept: code-423n4/2021-08-realitycards-findings#73

Recommendation:

Actual code :

function _withdraw() external {
        if (msg.sender != admin) {
            revert AdminOnly();
        }

        uint256 amount = address(this).balance;
        (bool sent, ) = msg.sender.call{ value: amount }('');

        emit Withdraw(amount, sent);
    }

Recommendation code :

function _withdraw() external {
        if (msg.sender != admin) {
            revert AdminOnly();
        }

        uint256 amount = address(this).balance;
        (bool sent, ) = msg.sender.call{ value: amount }('');
        require(sent, "Failed to send Ether");
        emit Withdraw(amount, sent);
    }

[Shungy]

1- It's a standalone function with sole purpose of withdrawal. It does not matter if it doesn't revert on failed withdrawal as it is not predicated on some other state change. #403 (comment)
2- The emit would be Withdraw(n, false), perfectly explaining that attempt to withdraw n amount of eth was failed.
3- It's a restricted function with no state change whatsoever. You don't need reentrancy guard. Do not blindly follow recommendations.
4- It's for withdrawing gas refund money, which would be put there by admins anyways. #45 (comment) #410 (comment)