dany.armstrong90 - `MinterGateway.sol#_verifyValidatorSignatures` function miscalculates `minTimestamp_`.

[sherlock-admin2]

dany.armstrong90

medium

MinterGateway.sol#_verifyValidatorSignatures function miscalculates minTimestamp_.

Summary

MinterGateway.sol#_verifyValidatorSignatures function calculates the minTimestamp_ to the minimum of the first threshold timestamps for which the signature is valid. It is not righteous to minters, and may cause to penalize a minter before the minter expires.

Vulnerability Detail

MinterGateway.sol#_verifyValidatorSignatures function is the following.

    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_);
            }
        }
    }

The above function calculates the minTimestamp_ to the minimum of the first threshold timestamps for which the signature is valid. The value will be changed depending on the order of timestamps. For instance, if timestamps is 1 o'clock, 1 o'clock, 0 o'clock, then minTimestamp_ = 1 o'clock holds, but if timestamps is 0 o'clock, 1 o'clock, 1 o'clock, minTimestamp_ = 0 o'clock holds true. It is disadvantageous to minters, and may cause to penalize a minter before the minter expires.

Example:

1. Assume that minter1 calls updateCollateral function with valid signatures from validator1 at 0 o'clock and from validator2 and validator3 at 1 o'clock.
2. Assume that updateCollateralValidatorThreshold() = 2.
3. updateCollateral function calculates as minTimestamp_ = 0 o'clock. 4, Since minter1 has two valid signatures at 1 o'clock, it should be calculated as minTimstamp_ = 1 o'clock. In that case, if updateCollateralInterval() = 12 hours, the minter1 needs only to call updateCollateral function before 13 o'clock.
4. But since minTimestamp_ = 0 o'clock, if minter1 doesn't call updateCollateral function untill 20th April, minter1 will be penalized which is not righteous.

Impact

Minter's updateTimestamp is miscalculated and so minter will be penalized which is not righteous.

Code Snippet

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

Tool used

Manual Review

Recommendation

We should sort the timestamps of valid signatures in the MinterGateway.sol#_verifyValidatorSignatures function in descending order and return the thresold-th timestamp.

Duplicate of #77

[nevillehuang]

See comment here