Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Missing check in `MarginCollateralConfiguration::configure` which leads to Arithmetic Underflow or Overflow in `MarginCollateralConfiguration::convertTokenAmountToUd60x18` and `MarginCollateralConfiguration::convertUd60x18ToTokenAmount`

spomaria

Summary

The MarginCollateralConfiguration::configure function is responsible for collateralType configuration such as setting the decimals of a collateral. This function is supposed to ensure that self.decimals <= Constants.SYSTEM_DECIMALS for all collaterals to conform with the protocol design.
However, there is no check(s) in the MarginCollateralConfiguration::configure function to ensure that self.decimals <= Constants.SYSTEM_DECIMALS for all collaterals.

Vulnerability Details

Because of the missing check(s) in the MarginCollateralConfiguration::configure function to ensure that self.decimals <= Constants.SYSTEM_DECIMALS for all collaterals as specified in the natspec of MarginCollateralConfiguration::convertTokenAmountToUd60x18, the MarginCollateralConfiguration::convertTokenAmountToUd60x18 and MarginCollateralConfiguration::convertUd60x18ToTokenAmount functions panic with an arithmetic underflow or overflow whenever self.decimals is greater than Constants.SYSTEM_DECIMALS, disrupting the protocol's functionality.

Note that the MarginCollateralConfiguration::configure function has no checks regarding the setting of decimals for collaterals as shown below:

function configure(
address collateralType,
uint128 depositCap,
uint120 loanToValue,
uint8 decimals,
address priceFeed,
uint32 priceFeedHearbeatSeconds
)
internal
{
Data storage self = load(collateralType);
​
self.depositCap = depositCap;
self.loanToValue = loanToValue;
self.decimals = decimals;
self.priceFeed = priceFeed;
self.priceFeedHearbeatSeconds = priceFeedHearbeatSeconds;
}

This results in an issue for the MarginCollateralConfiguration::convertTokenAmountToUd60x18 and MarginCollateralConfiguration::convertUd60x18ToTokenAmount functions that operate on the assumption that self.decimals <= Constants.SYSTEM_DECIMALS for all collaterals and therefore carries out their operations without running any checks.

/// @notice Converts the provided denormalized amount of margin collateral to UD60x18.
@> /// @dev We can assume self.decimals is always <= SYSTEM_DECIMALS, since it's a requirement at `setDecimals`.
/// @param self The margin collateral type storage pointer.
/// @param amount The amount of margin collateral to convert.
/// @return systemAmount The converted amount of margin collateral to the system's decimals.
​
function convertTokenAmountToUd60x18(Data storage self, uint256 amount) internal view returns (UD60x18) {
if (Constants.SYSTEM_DECIMALS == self.decimals) {
return ud60x18(amount);
}
return ud60x18(amount * 10 ** (Constants.SYSTEM_DECIMALS - self.decimals));
}
​
.
.
.
function convertUd60x18ToTokenAmount(Data storage self, UD60x18 ud60x18Amount) internal view returns (uint256) {
if (Constants.SYSTEM_DECIMALS == self.decimals) {
return ud60x18Amount.intoUint256();
}
​
return ud60x18Amount.intoUint256() / (10 ** (Constants.SYSTEM_DECIMALS - self.decimals));
}

Impact

In the natspec of MarginCollateralConfiguration::convertTokenAmountToUd60x18, it is specified that when setting decimals, the protocol ensures that self.decimals is always less than or equal to Constants.SYSTEM_DECIMALS for all collaterals. Since the MarginCollateralConfiguration::configure function sets all the parameters for a collateral but sets the decimals without making any checks to ensure that self.decimals is always less than or equal to Constants.SYSTEM_DECIMALS as specified in the natspec, this causes some issues for the MarginCollateralConfiguration::convertTokenAmountToUd60x18 and MarginCollateralConfiguration::convertUd60x18ToTokenAmount functions. Whenever when self.decimals is greater than Constants.SYSTEM_DECIMALS, the two functions panic with an arithmetic underflow or overflow instead of returning the converted values, thereby affecting the protocol's functionality.

Tools Used

Manual Review

Fuzz testing using foundry

Proof of Concept: Consider

  1. setting a collateral configuration using the MarginCollateralConfiguration::configure function such that decimals > Constants.SYSTEM_DECIMALS,

  2. then call the MarginCollateralConfiguration::convertTokenAmountToUd60x18 function which causes panic throwing an arithmetic overflow or underflow

  3. Also, calling the MarginCollateralConfiguration::convertUd60x18ToTokenAmount function causes panic throwing an arithmetic overflow or underflow

Proof of Code Place the following code into `convertTokenAmountToUd60x18.t.sol`.
function testFuzz_WhenMarginCollateralDecimalsIsGreaterThanSystemDecimals(
uint128 newDepositCap,
uint120 newLoanToValue,
uint8 newDecimals,
address newPriceFeed
)
external
{
uint256 amount = 100;
​
vm.assume(newDecimals > Constants.SYSTEM_DECIMALS && newDecimals > 0);
​
perpsEngine.exposed_configure(
address(usdc), newDepositCap, newLoanToValue, newDecimals, newPriceFeed, MOCK_PRICE_FEED_HEARTBEAT_SECONDS
);
​
UD60x18 expectedValue = ud60x18(amount * 10 ** (Constants.SYSTEM_DECIMALS - newDecimals));
​
UD60x18 value = perpsEngine.exposed_convertTokenAmountToUd60x18(address(usdc), amount);
​
// it should return the amount raised to the decimals of the system minus the decimals of the margin
// collateral to UD60x18
assertEq(value.intoUint256(), expectedValue.intoUint256(), "value is not correct");
}

Secondly, place the following code into convertUd60x18ToTokenAmount.t.sol.

function testFuzz_WhenMarginCollateralDecimalsIsGreaterThanSystemDecimals2(
uint128 newDepositCap,
uint120 newLoanToValue,
uint8 newDecimals,
address newPriceFeed
)
external
{
uint256 amount = 100;
​
vm.assume(newDecimals > Constants.SYSTEM_DECIMALS && newDecimals > 0);
​
perpsEngine.exposed_configure(
address(usdc), newDepositCap, newLoanToValue, newDecimals, newPriceFeed, MOCK_PRICE_FEED_HEARTBEAT_SECONDS
);
​
uint256 expectedValue = ((amount) / 10 ** (Constants.SYSTEM_DECIMALS - newDecimals));
​
uint256 value = perpsEngine.exposed_convertUd60x18ToTokenAmount(address(usdc), ud60x18(amount));
​
// it should return the amount raised to the decimals of the system minus the decimals of the margin
// collateral to uint256
assertEq(value, expectedValue, "value is not correct");
}

Recommendations

  1. Consider adding a check in the MarginCollateralConfiguration::configure function to ensure that decimals <= Constants.SYSTEM_DECIMALS during collateral configuration in order to avoid arithmetic overflow or underflow in the MarginCollateralConfiguration::convertTokenAmountToUd60x18 and MarginCollateralConfiguration::convertUd60x18ToTokenAmount functions. This way, the protocol's funtionality is preserved.

  2. Consider adding a custom error to use similar to TokensWithSuchDecimalsNotAllowed(string maxDecimal)

+ error TokensWithSuchDecimalsNotAllowed(string maxDecimal);
​
.
.
.
function configure(
address collateralType,
uint128 depositCap,
uint120 loanToValue,
uint8 decimals,
address priceFeed,
uint32 priceFeedHearbeatSeconds
)
internal
{
+ if(decimals > Constants.SYSTEM_DECIMALS) revert TokensWithSuchDecimalsNotAllowed("Max_Decimal_18");
Data storage self = load(collateralType);
​
self.depositCap = depositCap;
self.loanToValue = loanToValue;
self.decimals = decimals;
self.priceFeed = priceFeed;
self.priceFeedHearbeatSeconds = priceFeedHearbeatSeconds;
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Out of scope

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!