kevinkien - Missing validation of the value v in the _isValidSignature function.

[sherlock-admin4]

kevinkien

Medium

Missing validation of the value v in the _isValidSignature function.

Summary

The _isValidSignature function in the Ngt and SDAO contracts does not perform all necessary checks to verify EIP-712 signatures. Specifically, the function does not check the v value of the signature, which could lead to accepting invalid signatures in some cases.

Vulnerability Detail

The _isValidSignature function is used to verify signatures in the permit function, allowing users to authorize token spending without making an on-chain transaction. This function supports both EOA (Externally Owned Account) signatures via ecrecover and EIP-1271 style smart contract signatures.

However, during the EOA signature verification process, the function does not check the v value of the signature. The v value is an important part of the ECDSA signature, used to recover the signer's public address. Without checking v, an attacker could modify the signature to create a different but still valid signature, leading to unauthorized approvals or other unintended behaviors.

Impact

This vulnerability could allow an attacker to create forged signatures, bypassing the permit authorization mechanism and performing unauthorized actions such as transferring users' tokens without their consent. This could lead to loss of assets and damage the system's reputation.

Code Snippet

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/ngt/src/Ngt.sol#L180-L207

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/endgame-toolkit/src/SDAO.sol#L333-L358

    function _isValidSignature(
        address signer,
        bytes32 digest,
        bytes memory signature
    ) internal view returns (bool valid) {
        if (signature.length == 65) {
            bytes32 r;
            bytes32 s;
            uint8 v;
            assembly {
                r := mload(add(signature, 0x20))
                s := mload(add(signature, 0x40))
                v := byte(0, mload(add(signature, 0x60)))
            }
@>            if (signer == ecrecover(digest, v, r, s)) {
                return true;
            }
        }

        if (signer.code.length > 0) {
            (bool success, bytes memory result) = signer.staticcall(
                abi.encodeCall(IERC1271.isValidSignature, (digest, signature))
            );
            valid = (success &&
                result.length == 32 &&
                abi.decode(result, (bytes4)) == IERC1271.isValidSignature.selector);
        }
    }

Tool used

Manual Review

Recommendation

A check needs to be added to ensure that the v value of the signature is within the valid range (27 or 28):

    function _isValidSignature(address signer, bytes32 digest, bytes memory signature) internal view returns (bool) {
        if (signature.length == 65) {
            bytes32 r;
            bytes32 s;
            uint8 v;
            assembly {
                r := mload(add(signature, 0x20))
                s := mload(add(signature, 0x40))
                v := byte(0, mload(add(signature, 0x60)))
            }
+            if (signer == ecrecover(digest, v, r, s) && (v == 27 || v == 28)) {
                return true;
            }
        }

        if (signer.code.length > 0) {
            (bool success, bytes memory result) = signer.staticcall(
                abi.encodeCall(IERC1271.isValidSignature, (digest, signature))
            );
            return (success &&
                result.length == 32 &&
                abi.decode(result, (bytes4)) == IERC1271.isValidSignature.selector);
        }

        return false;
    }

[telome]

v is checked as part of ecrecover. Checking that v is in a specific range is unnecessary for permit() to function correctly. Also note that, as per the rules of the contest, "EIP incompatibilities are out of scope if they do not lead to direct loss of funds" and "There is no commitment with regards to EIP compliance. On top of that, any loss of funds incurred in an integrating contract is out of scope". Also note that signature malleability is not an issue for permit() as the nonce incrementation guarantees that a permit signature can only be used once as far as the token contract is concerned.