Signature can be forged for random addresses

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20MultiVotes.sol#L467-L505

Vulnerability details

Impact

A malicious user might attempt to forge signatures to delegate voting power from random addresses to delegatee.

Proof of Concept

During signature verification using ecrecover(), there exists a vulnerability where the signature can be forged, causing ecrecover() to return a random address. Consequently, the voting power of this random address might be delegated to delegate.
Malicious user can forge signatures to generated a large number of random addresses, then find out which one has non-zero balance and delegate its voting power without any permission.

Copy below codes to ERC20MultiVotes.t.sol and run forge test --match-test testDelegateWithForgedSig:

    function testDelegateWithForgedSig() public {
        token.setMaxDelegates(2);

        (uint8 v, bytes32 r, bytes32 s) = vm.sign(
            0x01,
            keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    token.DOMAIN_SEPARATOR(),
                    keccak256(
                        abi.encode(
                            token.DELEGATION_TYPEHASH(),
                            address(0x03),
                            0,
                            block.timestamp
                        )
                    )
                )
            )
        );

        //@audit-info voting power of address(0x03) is 0 before delegate forged signature
        assertEq(token.getVotes(address(0x03)), 0);
        for (uint i=0; i<100; i++) {
            //@audit-info simply increase 1 on s to forge new signature
            s = bytes32((uint256(s)+1));
            //@audit-info signer could be non-zero with forged signature
            address signer = ecrecover(
                keccak256(
                    abi.encodePacked(
                        "\x19\x01",
                        token.DOMAIN_SEPARATOR(),
                        keccak256(
                            abi.encode(
                                token.DELEGATION_TYPEHASH(),
                                address(0x03),
                                0,
                                block.timestamp
                            )
                        )
                    )
                ),
                v,
                r,
                s
            );
            if (signer != address(0)) {
                token.mint(signer, 100);
                token.delegateBySig(address(0x03), 0, block.timestamp, v, r, s);
                //@audit-info address(0x03) was delegated by random address signer 
                assertEq(token.containsDelegate(signer,address(0x03)), true);
            }
        }
        //@audit-info voting power of address(0x03) is not 0 after delegate forged signature as soon as the balance of forged random address is non-zero
        assertFalse(token.getVotes(address(0x03)) == 0);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Add delegator parameter in signed delegation, to avoid forging signatures for random addresses. Remove nonce parameter for simplicity:

    bytes32 public constant DELEGATION_TYPEHASH =
-       keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)");
+       keccak256("Delegation(address delegator,address delegatee,uint256 nonce,uint256 expiry)");

    /// @dev this consumes the same nonce as permit(), so the order of call matters.
    function delegateBySig(
+       address delegator,
        address delegatee,
-       uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public {
        require(
            block.timestamp <= expiry,
            "ERC20MultiVotes: signature expired"
        );
        address signer = ecrecover(
            keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    _domainSeparatorV4(),
                    keccak256(
                        abi.encode(
                            DELEGATION_TYPEHASH,
+                           delegator,
                            delegatee,
-                           nonce,
+                           _useNonce(delegator),
                            expiry
                        )
                    )
                )
            ),
            v,
            r,
            s
        );
-       require(nonce == _useNonce(signer), "ERC20MultiVotes: invalid nonce");
        require(signer != address(0));
+       require(signer == delegator, "ERC20MultiVotes: invalid signature");
        _delegate(signer, delegatee);
    }

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[eswak]

Malicious user can forge signatures to generated a large number of random addresses, then find out which one has non-zero balance and delegate its voting power without any permission.

Assuming there are 10,000 token holders with a significant GUILD balance to delegate, if I understand correctly, the odds of forging a signature that belongs to one of the top 10,000 holders is as hard as 1/10,000 the odds of finding a specific private key. And forging a signature for the a specific token holder should be as hard as finding their private key. I don't think there is an exploit vector here ?

[c4-sponsor]

eswak (sponsor) disputed

[Trumpero]

Invalid. If different addresses sign the same message, they will produce different signatures. signer is distinguished by different v, r, s.

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid