search

sherlock-audit / 2024-06-mellow-judging

8 stars 4 forks source link

Hunter - The `Vault` contract in the codebase does not handle tokens with different decimal places correctly, leading to potential inaccuracies in `lpAmount` calculations and valuation of tokens. #287

Closed sherlock-admin3 closed 4 months ago

sherlock-admin3 commented 5 months ago

Hunter

High

The Vault contract in the codebase does not handle tokens with different decimal places correctly, leading to potential inaccuracies in lpAmount calculations and valuation of tokens.

Summary

The Vault contract in the codebase does not handle tokens with different decimal places correctly, leading to potential inaccuracies in lpAmount calculations and valuation of tokens.

please note that this wasn't a problem and was in the known issues of the contest README but contest underlying tokens got updated to support USDC and USDT which introduced this problem

Vulnerability Detail

The Vault contract assumes that all tokens have the same number of decimals as the base token (ETH, with 18 decimals). However, when dealing with tokens like USDT, which has 6 decimals, the calculations for deposit and withdrawal values become inaccurate due to incorrect scaling.

The root cause of this issue lies in the deposit 

File: Vault.sol
285:     function deposit
            @>skipping some code
313:             uint256 ratioX96_ = FullMath.mulDiv(amounts[i], Q96, ratiosX96[i]);
314:             if (ratioX96_ < ratioX96) ratioX96 = ratioX96_;
315:         
316:         if (ratioX96 == 0) revert ValueZero();
317: 
318:         uint256 depositValue = 0;
319:         uint256 totalValue = 0;
320:         actualAmounts = new uint256[](tokens.length);
321:         IPriceOracle priceOracle = IPriceOracle(configurator.priceOracle());
322:         for (uint256 i = 0; i < tokens.length; i++) {
323:             uint256 priceX96 = priceOracle.priceX96(address(this), tokens[i]);
324:             totalValue += totalAmounts[i] == 0
325:                 ? 0
326:                 : FullMath.mulDivRoundingUp(totalAmounts[i], priceX96, Q96);
327:             if (ratiosX96[i] == 0) continue;
328:             uint256 amount = FullMath.mulDiv(ratioX96, ratiosX96[i], Q96);
329:             IERC20(tokens[i]).safeTransferFrom(
330:                 msg.sender,
331:                 address(this),
332:                 amount
333:             );
334:             actualAmounts[i] = amount;
335:             depositValue += FullMath.mulDiv(amount, priceX96, Q96);
336:         }

as we see in Line #335 depositValue passed to the _processLpAmount() are calculated using amount and priceX96

now we know that amounts hold different decimals in it for different tokens (USDT, USDC, rETH, wstETH,wETH) we can take a look at priceX96

File: ChainlinkOracle.sol
80:     function priceX96(
81:         address vault,
82:         address token
83:     ) external view returns (uint256 priceX96_) {
84:         if (vault == address(0)) revert AddressZero();
85:         if (token == address(0)) revert AddressZero();
86:         address baseToken = baseTokens[vault];
87:         if (baseToken == address(0)) revert AddressZero();
88:         if (token == baseToken) return Q96;
89:         (uint256 tokenPrice, uint8 decimals) = getPrice(vault, token);
90:         (uint256 baseTokenPrice, uint8 baseDecimals) = getPrice(
91:             vault,
92:             baseToken
93:         );
94:         priceX96_ = FullMath.mulDiv(
95:             tokenPrice * 10 ** baseDecimals,
96:             Q96,
97:             baseTokenPrice * 10 ** decimals
98:         );
99:     }

the function just fetches the prices from getPrice function that just calls _validateAndGetPrice that calls latestRoundData() every thing seems normal prices are fetched like usual

but the problem lies in the face that priceX96() function tries to scale and neutralize the difference between oracles decimals in Line #94 to #97 but doesn't take into account that fact that the price of low decimal tokens needs to be scaled for the vault accounting this leads to prices returned to be in oracle decimals what ever the decimals of the token

now back to depositValue in Vault.sol variable (we see that priceX96 always carry same decimals, Q96 is a constant, amount holds in it variable values according to variable decimals of token)

what this introduces is that a person that deposits 100K USDT (1e11) which in values equal almost 30ETH gets less lpAmount from a person supplying 1ETH (1e18) that is worth 3370 usd

Impact

The impact of this vulnerability depends on the specific tokens supported by the vault and their decimal places. If the vault supports tokens with different decimal the deposits calculations will be inaccurate, potentially leading to the following consequences:

generally impact will be high as this will cause loss of funds for users to uses to deposit large amounts of low decimal tokens and just deposit small amounts in other underlying

Code Snippet

Deposit() Function https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/src/Vault.sol#L285-L344 ```solidity File: Vault.sol 285: function deposit( 286: address to, 287: uint256[] memory amounts, 288: uint256 minLpAmount, 289: uint256 deadline 290: ) 291: external 292: nonReentrant 293: checkDeadline(deadline) 294: returns (uint256[] memory actualAmounts, uint256 lpAmount) 295: { 296: if (configurator.isDepositLocked()) revert Forbidden(); 297: IValidator(configurator.validator()).validate( 298: msg.sender, 299: address(this), 300: abi.encodeWithSelector(msg.sig) 301: ); 302: ( 303: address[] memory tokens, 304: uint256[] memory totalAmounts 305: ) = underlyingTvl(); 306: if (tokens.length != amounts.length) revert InvalidLength(); 307: uint128[] memory ratiosX96 = IRatiosOracle(configurator.ratiosOracle()) 308: .getTargetRatiosX96(address(this), true); 309: 310: uint256 ratioX96 = type(uint256).max; 311: for (uint256 i = 0; i < tokens.length; i++) { 312: if (ratiosX96[i] == 0) continue; 313: uint256 ratioX96_ = FullMath.mulDiv(amounts[i], Q96, ratiosX96[i]); 314: if (ratioX96_ < ratioX96) ratioX96 = ratioX96_; 315: } 316: if (ratioX96 == 0) revert ValueZero(); 317: 318: uint256 depositValue = 0; 319: uint256 totalValue = 0; 320: actualAmounts = new uint256[](tokens.length); 321: IPriceOracle priceOracle = IPriceOracle(configurator.priceOracle()); 322: for (uint256 i = 0; i < tokens.length; i++) { 323: uint256 priceX96 = priceOracle.priceX96(address(this), tokens[i]); 324: totalValue += totalAmounts[i] == 0 325: ? 0 326: : FullMath.mulDivRoundingUp(totalAmounts[i], priceX96, Q96); 327: if (ratiosX96[i] == 0) continue; 328: uint256 amount = FullMath.mulDiv(ratioX96, ratiosX96[i], Q96); 329: IERC20(tokens[i]).safeTransferFrom( 330: msg.sender, 331: address(this), 332: amount 333: ); 334: actualAmounts[i] = amount; 335: depositValue += FullMath.mulDiv(amount, priceX96, Q96); 336: } 337: 338: lpAmount = _processLpAmount(to, depositValue, totalValue, minLpAmount); 339: emit Deposit(to, actualAmounts, lpAmount); 340: address callback = configurator.depositCallback(); 341: if (callback == address(0)) return (actualAmounts, lpAmount); 342: IDepositCallback(callback).depositCallback(actualAmounts, lpAmount); 343: emit DepositCallback(callback, actualAmounts, lpAmount); 344: } ```

priceX96() function https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/src/oracles/ChainlinkOracle.sol#L80-L99 ```solidity File: ChainlinkOracle.sol 80: function priceX96( 81: address vault, 82: address token 83: ) external view returns (uint256 priceX96_) { 84: if (vault == address(0)) revert AddressZero(); 85: if (token == address(0)) revert AddressZero(); 86: address baseToken = baseTokens[vault]; 87: if (baseToken == address(0)) revert AddressZero(); 88: if (token == baseToken) return Q96; 89: (uint256 tokenPrice, uint8 decimals) = getPrice(vault, token); 90: (uint256 baseTokenPrice, uint8 baseDecimals) = getPrice( 91: vault, 92: baseToken 93: ); 94: priceX96_ = FullMath.mulDiv( 95: tokenPrice * 10 ** baseDecimals, 96: Q96, 97: baseTokenPrice * 10 ** decimals 98: ); 99: } ```

Tool used

manual review

Recommendation

the Vault contract should be modified to handle tokens with different decimal places correctly. One approach could be to introduce a scaling factor for each token based on the difference between its decimals and the needed decimal. This scaling factor would be applied either when calculating deposit and withdrawal values or when calculating the priceX96 inside ChainlinkOracle.sol, ensuring accurate calculations for all supported tokens, regardless of their decimal places.

Duplicate of #160