sherlock-audit / 2024-08-perennial-v2-update-3-judging

4 stars 2 forks source link

eeyore - Anyone can cancel other accounts `nonces` and `groups`, leading to griefing their `Intents`. #54

Open sherlock-admin4 opened 2 months ago

sherlock-admin4 commented 2 months ago

eeyore

Medium

Anyone can cancel other accounts nonces and groups, leading to griefing their Intents.

Summary

Within the AccountVerifier, OrderVerifier, and Verifier, anyone can call the verifyCommon() or cancelGroupWithSignature() functions with properly crafted data and signatures to cancel other users nonces or groups. This occurs because the signature is only compared to ensure the signer is the address from common.signer, but the cancellation is performed for common.account. There is no additional validation to confirm that common.signer is an allowed signer for the common.account.

Vulnerability Detail

As seen in the VerifierBase, the only check performed is signature validation against common.signer:

    function verifyCommon(Common calldata common, bytes calldata signature)
        external
        validateAndCancel(common, signature)
    {
@>      if (!SignatureChecker.isValidSignatureNow(common.signer, _hashTypedDataV4(CommonLib.hash(common)), signature))
            revert VerifierInvalidSignerError();
    }

In the validateAndCancel() modifier, the nonce is prechecked and later canceled for common.account without verifying that common.signer is an allowed signer for common.account:

    modifier validateAndCancel(Common calldata common, bytes calldata signature) {
        if (common.domain != msg.sender) revert VerifierInvalidDomainError();
        if (signature.length != 65) revert VerifierInvalidSignatureError();
        if (nonces[common.account][common.nonce]) revert VerifierInvalidNonceError();
        if (groups[common.account][common.group]) revert VerifierInvalidGroupError();
        if (block.timestamp >= common.expiry) revert VerifierInvalidExpiryError();

@>      _cancelNonce(common.account, common.nonce);

        _;
    }

As the same validation flow is used in cancelGroupWithSignature(), any group can be canceled as well.

This can lead to situations where:

The VerifierBase is used in all verifier contracts, affecting all functions that rely on Intents.

Impact

Code Snippet

https://github.com/sherlock-audit/2024-08-perennial-v2-update-3/blob/main/root/contracts/verifier/VerifierBase.sol#L18-L24
https://github.com/sherlock-audit/2024-08-perennial-v2-update-3/blob/main/root/contracts/verifier/VerifierBase.sol#L54-L57
https://github.com/sherlock-audit/2024-08-perennial-v2-update-3/blob/main/root/contracts/verifier/VerifierBase.sol#L76-L86

Tool used

Manual Review

Recommendation

Add additional validation to ensure that common.signer is an allowed signer for common.account.

sherlock-admin2 commented 1 month ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/equilibria-xyz/perennial-v2/pull/444 https://github.com/equilibria-xyz/root/pull/104

arjun-io commented 1 month ago

Note that in addition to the above PR, there is a 1-line update to the root package to enable this fix: https://github.com/equilibria-xyz/root/pull/104