MohammedRizwan - Incorrect value `BORROW_RATE_MAX_MANTISSA` used in contracts

[sherlock-admin4]

MohammedRizwan

High

Incorrect value BORROW_RATE_MAX_MANTISSA used in contracts

Summary

Incorrect value BORROW_RATE_MAX_MANTISSA used in contracts

Vulnerability Detail

Both UToken.sol and FixedInterestRateModel.sol has used the value of BORROW_RATE_MAX_MANTISSA as below:

    /**
     * @dev Maximum borrow rate that can ever be applied (.005% / 12 second)
     */
    uint256 internal constant BORROW_RATE_MAX_MANTISSA = 4_166_666_666_667; // 0.005e16 / 12

The issue is that, this calculated value by 0.005e16 / 12 is not correct. BORROW_RATE_MAX_MANTISSA is actually referenced from Compound's cToken which is implemented as below:

    // Maximum borrow rate that can ever be applied (.0005% / block)
    uint internal constant borrowRateMaxMantissa = 0.0005e16;

Note here that. the compound's Natspec for borrowRateMaxMantissa is not correct which was confirmed in openzeppelin's audit here. Instead of 0.0005%/ block, it should be 0.005%. Now coming back to issue, There is huge difference of values of compound's borrowRateMaxMantissa and currently implemented BORROW_RATE_MAX_MANTISSA in Union contracts.

After calculating the BORROW_RATE_MAX_MANTISSA in seconds:

1) Considering compound's borrowRateMaxMantissa = 0.0005e16 / 12 = 4_166_666_666_66

2) Considering currently implemented Union's BORROW_RATE_MAX_MANTISSA = 0.005e16 / 12 = 4_166_666_666_666

The difference is clearly of 3_750_000_000_000.

This would be an incorrect value of BORROW_RATE_MAX_MANTISSA and would allow to set the value of interestRatePerSecond.

The following functions are greatly affected by this issue:

    function setInterestRate(uint256 _interestRatePerSecond) external override onlyOwner {
@>        if (_interestRatePerSecond > BORROW_RATE_MAX_MANTISSA) revert BorrowRateExceeded();
        interestRatePerSecond = _interestRatePerSecond;

        emit LogNewInterestParams(_interestRatePerSecond);
    }

and

    function borrowRatePerSecond() public view override returns (uint256) {
        uint256 borrowRateMantissa = interestRateModel.getBorrowRate();
@>        if (borrowRateMantissa > BORROW_RATE_MAX_MANTISSA) revert BorrowRateExceedLimit();

        return borrowRateMantissa;
    }

borrowRatePerSecond() is further used in _calculatingInterest() and accrueInterest() functions and both of these functions have been extensively used across union contracts.

Another point is that, Hundred finance which is also deployed on optimism mainnet has used borrowRateMaxMantissa as below:

    uint internal constant borrowRateMaxMantissa = 0.00004e16;

Upon, further calculations, its concluded that 0.00004e16 (0.0005e16/12) is actually derived from Compound'sborrowRateMaxMantissawhich is0.0005e16. Since compound usesblock numberto calculate interest soborrowRateMaxMantissais calculated as0.0005e16/ blockand Hundred finance has usedblock timestampto calculate interest soborrowRateMaxMantissais calculated as0.0005e16/ secondtherefore,unionshould also follow same asHundred financeusedborrowRateMaxMantissa` on Optimisim mainnet.

Impact

BORROW_RATE_MAX_MANTISSA is the maximum borrow rate that can ever be applied in Union contracts has been used incorrectly. This would break the borrowRatePerSecond() function which is used to calculate the borrow rate and this borrow rate is fetched while calulating interest and acrueing interest. Since, it would result in huge difference as said above so this break a maximum borrow rate mantissa as referred from Compound.

Code Snippet

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/market/FixedInterestRateModel.sol#L18

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/market/UToken.sol#L74

Tool used

Manual Review

Recommendation

Consider calculating the BORROW_RATE_MAX_MANTISSA from 0.0005e16 instead of 0.005e16 due to as explained above.

Consider below changes in both UToken.sol and FixedInterestRateModel.sol:

    /**
-     * @dev Maximum borrow rate that can ever be applied (0.005% / 12 second)
+    * @dev Maximum borrow rate that can ever be applied (0.05% / 12 second)
     */
-    uint256 public constant BORROW_RATE_MAX_MANTISSA = 4_166_666_666_667; // 0.005e16 / 12
+    uint256 public constant BORROW_RATE_MAX_MANTISSA = 0.00004e16;                  // 0.0005e16 / 12

[sherlock-admin3]

Escalate I don't see how is this an issue. Every protocol can have its own rate plus it is never mentioned that rate is exact same as the one used by compound. The given natspec in the protocol states that they use (.005% / 12 second) as the BORROW_RATE_MAX_MANTISSA and it is correctly calculated. Natspec and the calculated value match thus invalid issue.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[vsharma4394]

Escalate I don't see how is this an issue. Every protocol can have its own rate plus it is never mentioned that rate is exact same as the one used by compound. The given natspec in the protocol states that they use (.005% / 12 second) as the BORROW_RATE_MAX_MANTISSA and it is correctly calculated. Natspec and the calculated value match thus invalid issue.

[sherlock-admin3]

Escalate I don't see how is this an issue. Every protocol can have its own rate plus it is never mentioned that rate is exact same as the one used by compound. The given natspec in the protocol states that they use (.005% / 12 second) as the BORROW_RATE_MAX_MANTISSA and it is correctly calculated. Natspec and the calculated value match thus invalid issue.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[c-plus-plus-equals-c-plus-one]

I think @vsharma4394 is right. This BORROW_RATE_MAX_MANTISSA is the same as it was in the pre-update Union Finance codebase.

[0xRizwan]

Union's UToken and interestRate model contract resemebles with Compound's contracts.

For everychain, BORROW_RATE_MAX_MANTISSA has different value. Check this reference. Since Compound had used block number so the value was .0005% / block.

Now, coming to Union's comment on BORROW_RATE_MAX_MANTISSA:

    /**
     * @dev Maximum borrow rate that can ever be applied (.005% / 12 second)
     */
    uint256 internal constant BORROW_RATE_MAX_MANTISSA = 4_166_666_666_667; // 0.005e16 / 12

For calculations, it has taken .005% in 0.005e16 / 12 which is incorrect as the interest rate is being calculated at 1e18 base.

So, if we see the calculations i.e 4_166_666_666_667; // 0.005e16 / 12, it has taken 0.005e16 which means it considers 0.5% and not 0.005% as per comments.

So, this proves the calculations of BORROW_RATE_MAX_MANTISSA is incorrect.

Union is supposed to deploy contracts on Optimism and as i said above, BORROW_RATE_MAX_MANTISSA differs for every chain. Hundred finance which is also deployed on Optimism has used 0.00004e16 as BORROW_RATE_MAX_MANTISSA which is actually correctly derived from Compound's borrowRateMaxMantissa.

Hope, this clears the issue.

On escalation, i would accept final decision by sherlock HOJ.

[vsharma4394]

0.005% is equal to 0.005e16 i don't get it why are you saying it is equal to 0.5% . 0.5% would be 0.5e16. So the calculations are correct. Secondly there's been no mention in the readme Or anywhere in the comments that they use the compounds model of using interest rate based on the block like how compound is using. In the comments it is clear that the rate used is (0.005%/12 seconds). Had the comments been (0.005%/ block) finding would be valid but as of now it is invalid.

[WangSecurity]

The above comment is correct and 0.005% indeed equals 0.005e16. Hence, the comment matches the actual value. Additionally, even though it's a fork of Compound it doesn't mean they have to use the exact same values or the values of another fork.

Planning to accept the escalation and invalidate the report.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• vsharma4394: accepted