tx.origin may be removed in future and its usage is not recommended

[code423n4]

Lines of code

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOLogicV2.sol#L1043

Vulnerability details

Impact

There is a chance that tx.origin will be removed from the Ethereum protocol in the future, so code that uses tx.origin must be avoid using it.

There is also some EIPs being proposed for change/remove of tx.origin.

ethereum/EIPs#637

https://www.reddit.com/r/ethereum/comments/6d11lv/erc_about_txorigin_change_for_account_abstraction/

In NounsDAOLogicV2.sol,

File: nouns-contracts/contracts/governance/NounsDAOLogicV2.sol

1033    function _refundGas(uint256 startGas) internal {
1034        unchecked {
1035            uint256 balance = address(this).balance;
1036            if (balance == 0) {
1037                return;
1038            }
1039            uint256 basefee = min(block.basefee, MAX_REFUND_BASE_FEE);
1040            uint256 gasPrice = min(tx.gasprice, basefee + MAX_REFUND_PRIORITY_FEE);
1041            uint256 gasUsed = min(startGas - gasleft() + REFUND_BASE_GAS, MAX_REFUND_GAS_USED);
1042            uint256 refundAmount = min(gasPrice * gasUsed, balance);
1043            (bool refundSent, ) = tx.origin.call{ value: refundAmount }('');
1044            emit RefundableVote(tx.origin, refundAmount, refundSent);
1045        }
1046    }

Here, the function has used tx.origin as recipient to refund the amount but these can not work in future if the new EIPs concerning to remove tx.origin removal due to security issues got approved and tx.origin is removed from Ethereum protocol.

Proof of Concept

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOLogicV2.sol#L1043

For example:
In NounsDAOV3Votes.sol, the same function logic has used msg.sender instead of tx.origin. The same should be considered in NounsDAOLogicV2.sol,

File: nouns-contracts/contracts/governance/NounsDAOV3Votes.sol

    function _refundGas(uint256 startGas) internal {
        unchecked {
            uint256 balance = address(this).balance;
            if (balance == 0) {
                return;
            }
            uint256 basefee = min(block.basefee, MAX_REFUND_BASE_FEE);
            uint256 gasPrice = min(tx.gasprice, basefee + MAX_REFUND_PRIORITY_FEE);
            uint256 gasUsed = min(startGas - gasleft() + REFUND_BASE_GAS, MAX_REFUND_GAS_USED);
            uint256 refundAmount = min(gasPrice * gasUsed, balance);
>>          (bool refundSent, ) = msg.sender.call{ value: refundAmount }('');
            emit RefundableVote(msg.sender, refundAmount, refundSent);
        }
    }

Tools Used

Manual review

Recommended Mitigation Steps

Use msg.sender instead of tx.origin.

File: nouns-contracts/contracts/governance/NounsDAOLogicV2.sol

    function _refundGas(uint256 startGas) internal {
        unchecked {
            uint256 balance = address(this).balance;
            if (balance == 0) {
                return;
            }
            uint256 basefee = min(block.basefee, MAX_REFUND_BASE_FEE);
            uint256 gasPrice = min(tx.gasprice, basefee + MAX_REFUND_PRIORITY_FEE);
            uint256 gasUsed = min(startGas - gasleft() + REFUND_BASE_GAS, MAX_REFUND_GAS_USED);
            uint256 refundAmount = min(gasPrice * gasUsed, balance);
-            (bool refundSent, ) = tx.origin.call{ value: refundAmount }('');
+            (bool refundSent, ) = msg.sender.call{ value: refundAmount }('');
            emit RefundableVote(tx.origin, refundAmount, refundSent);
        }
    }

Assessed type

Other

[0xSorryNotSorry]

OOS --> [LOW‑3] Avoid using tx.origin

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Out of scope