code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/governance/GovernorBravoDelegateMaia.sol#L344-L352

Vulnerability details

Impact

In GovernorBravoDelegateMaia.sol,

File: src/governance/GovernorBravoDelegateMaia.sol

343    function castVoteBySig(uint256 proposalId, uint8 support, uint8 v, bytes32 r, bytes32 s) external {
344        bytes32 domainSeparator =
345            keccak256(abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainIdInternal(), address(this)));
346        bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
347        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
348        address signatory = ecrecover(digest, v, r, s);
349        require(signatory != address(0), "GovernorBravo::castVoteBySig: invalid signature");
350        emit VoteCast(signatory, proposalId, support, castVoteInternal(signatory, proposalId, support), "");
351    }

The castVoteBySig() is used for Casting a vote for a proposal by signature. This function accepts EIP-712 signatures for voting on proposals. The issue here is Nonce is missing here in signed data. A nonce can prevent an old value from being used when a new value exists. Without one, two transactions submitted in one order, can appear in a block in a different order.

EIP-712 has stressed on security issues like Replay attacks which is possible here and Frontrunning attacks. Therefore all possible measures must be taken to prevent signature replay attacks i.e by providing incremented nonce in signed data.

EIP-712 security consideration link- https://eips.ethereum.org/EIPS/eip-712

To understand how signature replay attacks work, Please refer chapter link from Mastering Ethereum book, https://github.com/ethereumbook/ethereumbook/blob/develop/06transactions.asciidoc

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/governance/GovernorBravoDelegateMaia.sol#L344-L352

References

Reference 1:- Referrencing here openzeppelin Governor.sol for understanding and mitigation implementation, In openzeppelin Governor.sol,

File: contracts/governance/Governor.sol

518    function castVoteBySig(
519        uint256 proposalId,
520        uint8 support,
521        address voter,
522        uint8 v,
523        bytes32 r,
524        bytes32 s
525    ) public virtual override returns (uint256) {
526        address signer = ECDSA.recover(
527            _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, _useNonce(voter)))),
528            v,
529            r,
530            s
        );

        // some code
    }

check,

527            _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, _useNonce(voter)))),

See how nonce is incremented here using _useNonce(..) to prevent signature replay attacks.

Reference link

Reference 2:- In ERC20MultiVotes.sol, delegateBySig() function is prevented by signature replay attacks as this function has incremented nonce in signed data.

Reference link

Tools Used

Manual review

Recommended Mitigation Steps

Consider the incremented nonce in signed data to prevent signature replay attacks. Refer the given both references for the similar implementation to mitigate this issue.

Assessed type

Other

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Invalid

trust1995 commented 1 year ago

Because it signs a unique proposalId, it does not need replay protection.

0xRizwan commented 1 year ago

@trust1995 Ser,

With due respect to decision, I think the hash data should be provided with incremented nonce.

I crossed check with openzeppelin castVoteBySig() implementation of Governor.sol, There incremented nonce is present in hash data.

Reference: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6e214227376acd688e60fcdca8cb7f0ae8cef98f/contracts/governance/Governor.sol#L527

Request to have a look.

Thank you!