search

sherlock-audit / 2023-10-mzero-judging

3 stars 2 forks source link

FastTiger - The `MinterGateway.sol#_verifyvalidatorSignatures` function calculates `minTimestamp` incorrectly. #77

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

FastTiger

medium

The MinterGateway.sol#_verifyvalidatorSignatures function calculates minTimestamp incorrectly.

Summary

TheMinterGateway.sol#_verifyvalidatorSignatures function returns minTimestamp_ as the minimum value of the first updateCollateralValidatorThreshold valid timestamps in the signature_ array, regardless of the total number of signatures entered. This can be detrimental to minters when they use more signatures than the updateCollateralValidatorThreshold number and the minter's updateTimestamp is pulled.

Vulnerability Detail

The MinterGateway.sol#_verifyvalidatorSignatures function is as follows.

    function _verifyValidatorSignatures(
        address minter_,
        uint256 collateral_,
        uint256[] calldata retrievalIds_,
        bytes32 metadataHash_,
        address[] calldata validators_,
        uint256[] calldata timestamps_,
        bytes[] calldata signatures_
    ) internal view returns (uint40 minTimestamp_) {
        uint256 threshold_ = updateCollateralValidatorThreshold();

        minTimestamp_ = uint40(block.timestamp);

        // Stop processing if there are no more signatures or `threshold_` is reached.
        for (uint256 index_; index_ < signatures_.length && threshold_ > 0; ++index_) {
            unchecked {
                // Check that validator address is unique and not accounted for
                // NOTE: We revert here because this failure is entirely within the minter's control.
                if (index_ > 0 && validators_[index_] <= validators_[index_ - 1]) revert InvalidSignatureOrder();
            }

            // Check that validator is approved by TTG.
            if (!isValidatorApprovedByTTG(validators_[index_])) continue;

            // Check that the timestamp is not 0.
            if (timestamps_[index_] == 0) revert ZeroTimestamp();

            // Check that the timestamp is not in the future.
            if (timestamps_[index_] > uint40(block.timestamp)) revert FutureTimestamp();

            // NOTE: Need to store the variable here to avoid a stack too deep error.
            bytes32 digest_ = _getUpdateCollateralDigest(
                minter_,
                collateral_,
                retrievalIds_,
                metadataHash_,
                timestamps_[index_]
            );

            // Check that ECDSA or ERC1271 signatures for given digest are valid.
            if (!SignatureChecker.isValidSignature(validators_[index_], digest_, signatures_[index_])) continue;

            // Find minimum between all valid timestamps for valid signatures.
            minTimestamp_ = UIntMath.min40(minTimestamp_, uint40(timestamps_[index_]));

            unchecked {
                --threshold_;
            }
        }

        // NOTE: Due to STACK_TOO_DEEP issues, we need to refetch `requiredThreshold_` and compute the number of valid
        //       signatures here, in order to emit the correct error message. However, the code will only reach this
        //       point to inevitably revert, so the gas cost is not much of a concern.
        if (threshold_ > 0) {
            uint256 requiredThreshold_ = updateCollateralValidatorThreshold();

            unchecked {
                // NOTE: By this point, it is already established that `threshold_` is less than `requiredThreshold_`.
                revert NotEnoughValidSignatures(requiredThreshold_ - threshold_, requiredThreshold_);
            }
        }
    }
    }

This function computes minTimestamp_ as the minimum of the first valid updateCollateralValidatorThreshold timestamps in the signatures_ array. In fact, this is disadvantageous to minters. Let's look at an example when updateCollateralValidatorThreshold() = 2. If the four timestamps are 5,6,2,1 and all four are valid, then min(5,6) = 5, so minTimestamp_ = 5. In fact, in this case, it is more accurate to set minTimestamp_ = 1 rather than minTimestamp_ = 5. minters do not change the order of the four timestamps from 5,6,2,1 to 1,2,5,6. Because validators_must be arranged in order, the order cannot be changed. Therefore, the current MinterGateway.sol#_verifyValidatorSignatures function is unfavorable to Minter and causes minters anxiety.

Impact

Users feel resistance to this protocol because they incur unexpected losses.

Code Snippet

https://github.com/sherlock-audit/2023-10-mzero/blob/main/protocol/src/MinterGateway.sol#L1045-L1107

Tool used

Manual Review

Recommendation

Modify the MinterGateway.sol#_verifyvalidatorSignatures function as follows.

    function _verifyValidatorSignatures(
        address minter_,
        uint256 collateral_,
        uint256[] calldata retrievalIds_,
        bytes32 metadataHash_,
        address[] calldata validators_,
        uint256[] calldata timestamps_,
        bytes[] calldata signatures_
    ) internal view returns (uint40 minTimestamp_) {
        uint256 threshold_ = updateCollateralValidatorThreshold();

        minTimestamp_ = uint40(block.timestamp);

        // Stop processing if there are no more signatures or `threshold_` is reached.
-       for (uint256 index_; index_ < signatures_.length && threshold_ > 0; ++index_) {
+       for (uint256 index_; index_ < signatures_.length; ++index_) {
            unchecked {
                // Check that validator address is unique and not accounted for
                // NOTE: We revert here because this failure is entirely within the minter's control.
                if (index_ > 0 && validators_[index_] <= validators_[index_ - 1]) revert InvalidSignatureOrder();
            }

            // Check that validator is approved by TTG.
            if (!isValidatorApprovedByTTG(validators_[index_])) continue;

            // Check that the timestamp is not 0.
            if (timestamps_[index_] == 0) revert ZeroTimestamp();

            // Check that the timestamp is not in the future.
            if (timestamps_[index_] > uint40(block.timestamp)) revert FutureTimestamp();

            // NOTE: Need to store the variable here to avoid a stack too deep error.
            bytes32 digest_ = _getUpdateCollateralDigest(
                minter_,
                collateral_,
                retrievalIds_,
                metadataHash_,
                timestamps_[index_]
            );

            // Check that ECDSA or ERC1271 signatures for given digest are valid.
            if (!SignatureChecker.isValidSignature(validators_[index_], digest_, signatures_[index_])) continue;

            // Find minimum between all valid timestamps for valid signatures.
            minTimestamp_ = UIntMath.min40(minTimestamp_, uint40(timestamps_[index_]));

+           unchecked {
+               --threshold_;
+           }
        }

        // NOTE: Due to STACK_TOO_DEEP issues, we need to refetch `requiredThreshold_` and compute the number of valid
        //       signatures here, in order to emit the correct error message. However, the code will only reach this
        //       point to inevitably revert, so the gas cost is not much of a concern.
-       if (threshold_ > 0) {
+       if (threshold_ - signatures_.length > 0) {
            uint256 requiredThreshold_ = updateCollateralValidatorThreshold();

            unchecked {
                // NOTE: By this point, it is already established that `threshold_` is less than `requiredThreshold_`.
                revert NotEnoughValidSignatures(requiredThreshold_ - threshold_, requiredThreshold_);
            }
        }
    }
PierrickGT commented 7 months ago

Acknowledged.

Minters are entirely in control of the signatures they provide and should not provide more signatures than needed. For this reason, we won't implement the code suggestion.

FastTiger777 commented 7 months ago

I do not think so. If so, when signatures that minters suggest are bigger than threshold, contract must be reverted. If not so, attacker can use this.

deluca-mike commented 7 months ago

Thus is not really an issue.

First, I'm confused why the warden believes larger timestamps are unfavorable, rather than smaller ones. A smaller min timestamp is actually unfavorable as it means the collateral update is more outdated, and the next update will be required sooner, or else penalties will apply sooner.

That misunderstanding aside, the issue, as described, still applies if we take "unfavorable" to actually mean smaller, and thus using the example 2,1,5,6.

But, this isn't an issue.

If a subset of the provided signatures resulted in an "unfavorable" min timestamp (i.e. the smallest one provided, thus the earliest required next collateral update), then you can simply resubmit the collateral update without that smallest timestamp signature, and you'd still meet the validator threshold, and would not get a later min timestamp.

FastTiger777 commented 7 months ago

Since the transaction has been completed, I don't think there is any way to reverse it.

deluca-mike commented 7 months ago

There's no way to reverse it, but simply calling the function again with a smaller set of valid signatures will result in the collateral being updated again with the larger min timestamp, assuming you remove the smallest timestamp from the set of excess signatures.

So if you only need three, and you provide 5,6,1,2, your new on-chain collateral time stamp will be 1. If you're kicking yourself for having included the signature with the timestamp of 1, you can simply create a new transaction providing only 5,6,2, which meets the threshold and will result in your onchain collateral timestamp being 2.

FastTiger777 commented 7 months ago

I know what you mean. However, if that happens, users will feel resistance to the protocol. And attacker can use this to get unfair profit.

On Mon, Apr 1, 2024 at 10:02 PM Michael De Luca @.***> wrote:

There's no way to reverse it, but simply calling the function again with a smaller set of valid signatures will result in the collateral being updated again with the larger min timestamp, assuming you remove the smallest timestamp from the set of excess signatures.

So if you only need three, and you provide 5,6,1,2, your new on-chain collateral time stamp will be 1. If you're kicking yourself for having included the signature with the time stamp of one, you can simply create a new transaction providing only 5,6,2, which meets the threshold and will result in your unchained collateral timestamp being 2.

— Reply to this email directly, view it on GitHub https://github.com/sherlock-audit/2023-10-mzero-judging/issues/77#issuecomment-2029725381, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFGF2S6ZPN6EF5JXTQ423VLY3FLGHAVCNFSM6AAAAABFLCVHKCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRZG4ZDKMZYGE . You are receiving this because you commented.Message ID: @.***>

deluca-mike commented 7 months ago

Sorry, I don't follow. How can an attacker use this to profit? Who is profiting, and how?

The collateral update accepts more validators than the threshold to protect minters by allowing them to exceed requirements in case on chain requirements change, or validators are removed, while their transaction is in flight or in the mempool. This is because there are penalties for a late collateral update.

FastTiger777 commented 7 months ago

You agreed that there is no way to reverse it. so users will feel resistance to the protocol. Is this right? if right, it's DOS.

On Tue, Apr 2, 2024 at 1:12 AM Michael De Luca @.***> wrote:

Sorry, I don't follow. How can an attacker use this to profit? Who is profiting, and how?

The collateral update accepts more validators then the threshold to protect minters by allowing them to exceed requirements in case on chain requirements change, or validators are removed, while their transaction is in flight or in the mempool. This is because there are penalties for a late collateral update.

— Reply to this email directly, view it on GitHub https://github.com/sherlock-audit/2023-10-mzero-judging/issues/77#issuecomment-2030073228, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFGF2SY45IDRJW643BDELQTY3GBOHAVCNFSM6AAAAABFLCVHKCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZQGA3TGMRSHA . You are receiving this because you commented.Message ID: @.***>

deluca-mike commented 7 months ago

There is a way to reverse the effects: broadcast a new transaction with the subset of timestamps you actually want. I don't understand the "users will feel resistance to the protocol". What resistance? Also, how can an attacker use this to profit? Who is profiting, and how?

FastTiger777 commented 7 months ago

you say that "There's no way to reverse it, but simply calling the function again with a smaller set of valid signatures will result in the collateral being updated again with the larger min timestamp, assuming you remove the smallest timestamp from the set of excess signatures.". I think users wanna more simple and more exact and more credit transaction.

deluca-mike commented 7 months ago

There seems to be a conflation of terminology here. There is no way to reverse an Ethereum transaction. There is a way to reverse the effects of over-providing signatures in a way that results in a smaller min timestamp than one deems favourable.

A minter will provide more signatures than necessary if they feel it to be prudent to do so, such as if:

Keep in mind, with how quick Emergency Proposals can be created, voted on, and executed, these are very valid concerns.

So if a minter goes out and collects 4 signatures when they only need 3, it's likely that these signatures all differ in timestamp by at most 2 minutes? 10 minutes?

They then update their collateral, and now only need to update collateral again in an updateInterval after the smallest of the first 3 timestamps. If they see they could have pushed that update requirement a few more minutes into the future by updating their collateral again and dropping the signature with the smallest timestamp, they can do that without any harm. If the transaction goes through, great. If it doesn't, its fine, because the important thing is that their original transaction went through.

The ability to over-provide signatures is a mechanism for minters to make a conscious tradeoff between the success of their updateCollateral call and the (very tiny) repercussions of providing more signatures than necessary.

And keep in mind, minters aren't your run-of-the-mill user, they are institutions.

toninorair commented 7 months ago

We allow more signatures than the threshold to avoid discrepancies that can happen if the threshold is changed by TTG, the validator is removed from the list of approved validators (for ex - decreases) while the minter’s transaction is still pending.

Minter is in charge of their collateral validations and will aim to provide the min acceptable threshold of validations.

FastTiger777 commented 7 months ago

I do not understand what you mean. Is there something wrong with our scenario?

PierrickGT commented 7 months ago

I do not understand what you mean. Is there something wrong with our scenario?

Nothing wrong with your scenario but we assume that the Minters are in control of the signatures they provide, so they can order them properly to use the appropriate min timestamp.

FastTiger777 commented 7 months ago

But that's an assumption and that's exactly what happens, right?

On Wed, Apr 3, 2024 at 8:07 AM Pierrick Turelier @.***> wrote:

I do not understand what you mean. Is there something wrong with our scenario?

Nothing wrong with your scenario but we assume that the Minters are in control of the signatures they provide, so they can order them properly to use the appropriate min timestamp.

— Reply to this email directly, view it on GitHub https://github.com/sherlock-audit/2023-10-mzero-judging/issues/77#issuecomment-2033260898, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFGF2S4EXIQYNF226W6YKWTY3M2Z3AVCNFSM6AAAAABFLCVHKCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZTGI3DAOBZHA . You are receiving this because you commented.Message ID: @.***>

nevillehuang commented 7 months ago

Based on discussions, and the following point, I believe this issue is invalid and would constitute user input error.

The ability to over-provide signatures is a mechanism for minters to make a conscious tradeoff between the success of their updateCollateral call and the (very tiny) repercussions of providing more signatures than necessary.