sherlock-audit / 2023-10-mzero-judging

3 stars 2 forks source link

coffiasd - The current rate should revert instead of returning a zero value #41

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

coffiasd

medium

The current rate should revert instead of returning a zero value

Summary

Before updating the index, the protocol invokes _rate() to retrieve the current value of the rate, and then records the value of the rate to _latestRate. The issue is that the staticcall can revert due to situations such as:

Vulnerability Detail

From StableEarnerRateModel.sol we can see: https://github.com/sherlock-audit/2023-10-mzero/blob/main/protocol/src/rateModels/StableEarnerRateModel.sol#L83-L143

        if (totalActiveOwedM_ <= totalEarningSupply_) {
            // NOTE: `totalActiveOwedM_ * minterRate_` can revert due to overflow, so in some distant future, a new
            //       rate model contract may be needed that handles this differently.
            return uint32((uint256(totalActiveOwedM_) * minterRate_) / totalEarningSupply_);
        }

        // NOTE: `totalActiveOwedM_ * deltaMinterIndex_` can revert due to overflow, so in some distant future, a new
        //       rate model contract may be needed that handles this differently.
        int256 lnArg_ = int256(
            _EXP_SCALED_ONE +
                ((uint256(totalActiveOwedM_) * (deltaMinterIndex_ - _EXP_SCALED_ONE)) / totalEarningSupply_)
        );

it is possible for getSafeEarnerRate to revert.

In the second situation, if ttg configures an incorrect rateModel address, the _rate function can revert Once staticcall revert _rate return a zero value instead of revert this can lead to protocol record a incorrect zero _latestRate.

Since we use _latestRate to calculate currentIndex https://github.com/sherlock-audit/2023-10-mzero/blob/main/protocol/src/MToken.sol#L158-L173

    function currentIndex() public view override(ContinuousIndexing, IContinuousIndexing) returns (uint128) {//@audit-info calculate according to last update timestamp.
        // NOTE: Safe to use unchecked here, since `block.timestamp` is always greater than `latestUpdateTimestamp`.
        unchecked {
            return
                // NOTE: Cap the index to `type(uint128).max` to prevent overflow in present value math.
                UIntMath.bound128(
                    ContinuousIndexingMath.multiplyIndicesDown(
                        latestIndex,
                        ContinuousIndexingMath.getContinuousIndex(
                            ContinuousIndexingMath.convertFromBasisPoints(_latestRate),
                            uint32(block.timestamp - latestUpdateTimestamp)//@audit-info according to last updateTimestamp.
                        )
                    )
                );
        }
    }

Additionally, if currentIndex is used to calculate user funds, it may result in an error in the calculation of the user's funds

Impact

the calculation of the user's funds is error

Code Snippet

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

Tool used

Manual Review

Recommendation

@@ -452,7 +452,7 @@ contract MToken is IMToken, ContinuousIndexing, ERC20Extended {
             abi.encodeWithSelector(IRateModel.rate.selector)
         );

-        rate_ = (success_ && returnData_.length >= 32) ? UIntMath.bound32(abi.decode(returnData_, (uint256))) : 0;
+        rate_ = (success_ && returnData_.length >= 32) ? UIntMath.bound32(abi.decode(returnData_, (uint256))) : revert IncorrectRate(); 
     }

Duplicate of #48

sherlock-admin3 commented 7 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid; medium(3)