valentin2304 - `checkSignature` modifier is implemented wrongly

[sherlock-admin4]

valentin2304

medium

checkSignature modifier is implemented wrongly

Medium

Summary

checkSignature modifier in TitlesGraph is responsible for verifying user's signatures, but is vulnerable to replay attack because of wrong implementation.

Vulnerability Detail

    modifier checkSignature(bytes32 edgeId, bytes calldata data, bytes calldata signature) {
        bytes32 digest = _hashTypedData(keccak256(abi.encode(ACK_TYPEHASH, edgeId, data)));
        if (
            !edges[edgeId].to.creator.target.isValidSignatureNowCalldata(digest, signature)
                || _isUsed[keccak256(signature)]
        ) {
            revert Unauthorized();
        }
        _;
        _isUsed[keccak256(signature)] = true;
    }

_isUsed[keccak256(signature)] mapping is used to prevent signature replay but is getting updated after code execution which can lead to replay attack in case somebody calls another function that needs this signature via reentrancy. _isUsed[keccak256(signature)] is not following CEI pattern and this can lead to loss of funds. _isUsed[keccak256(signature)] should be updated before code execution.

Impact

Signature replay

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/graph/TitlesGraph.sol#L40-#L50

Tool used

Manual Review

Recommendation

    modifier checkSignature(bytes32 edgeId, bytes calldata data, bytes calldata signature) {
        bytes32 digest = _hashTypedData(keccak256(abi.encode(ACK_TYPEHASH, edgeId, data)));
        if (
            !edges[edgeId].to.creator.target.isValidSignatureNowCalldata(digest, signature)
                || _isUsed[keccak256(signature)]
        ) {
            revert Unauthorized();
        }
        _isUsed[keccak256(signature)] = true;
        _;
    }

change checkSignature modifier to the code above

[ccashwell]

There are no funds involved here, no loss of funds is possible. The only possible re-entry here is if the (...).target.isValidSignatureNowCalldata() which itself isn't a concern because it's only possible with a valid signature in the first place, which implies that the caller is authorized to modify the contract state they're touching. Won't fix.