User will get a smaller amount because of wrong calculation in weightedTokenAmount in the withdrawAll function

[hats-bug-reporter[bot]]

Github username: -- Twitter username: 97Sabit Submission hash (on-chain): 0x34bb6819adbf227d9290469fc4bd3380e2a4400187e98a3b7e0c762524285775 Severity: high

Description: Description\ In the withdrawAll function, weightedTokenAmount is divided by weight[token]:

unchecked { // remove the weight from weightedTokenAmount. weightedTokenAmount /= _weight[token]; }

This is not suppose not to be. Even based on the comment, weight is supposed to be removed from weightedTokenAmount and not divided by it as it is in the code.

Consequently, the weightedTokenAmount is transferred to a user.

This means that a user will get smaller tokens.

1. Proof of Concept (PoC) File

   https://github.com/catalystdao/catalyst/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystVaultAmplified.sol#L593-L596

2. Revised Code File (Optional)

[reednaa]

For the amplified vault weights are used to change the decimals (and price) not token distribution. This implies that they are directly multiplied on. This ensures the curve is distributed evenly on both sides of the price.

This implies the weights are multiplied.

For example, look at the documentation here: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/IntegralsAmplified.sol#L13-L15

And how the weight is used here: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/IntegralsAmplified.sol#L32-L34

Or right below: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/IntegralsAmplified.sol#L37-L38

Or for deposits here: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystVaultAmplified.sol#L329

[ololade97]

You didn't address this:

unchecked { // remove the weight from weightedTokenAmount. weightedTokenAmount /= _weight[token]; }

The comment and the code are contradictory.

[reednaa]

The weight is removed by division. (scalars are removed by dividing by the scalar)

[ololade97]

For example, weightedTokenAmount is 10. weight[token] is 2.

By this: weightedTokenAmount /= _weight[token];

It means weightedTokenAmount = 10/2 5 is stored in weightedTokenAmount and sent to the caller.

Besides, remove the weight from weightedTokenAmount is translated differently by you.

[reednaa]

Lets do a real example. You have DAI and USDC in an amplified vault.

DAI has 18 decimals while USDC has 6. So we set USDC to a weight of 10^(18-6) = 10^12.

So before we do any calculation on USDC we need to multiply it with 10^12.

That means, when the amount is calculated here: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystVaultAmplified.sol#L578-L587 We get 1000 USDC as 1000 10^6 (decmials) 10^12 (weight).

We want to send the user 1000 USDC (including decimals) so to remove the weight we need to subtract it. So we get: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystVaultAmplified.sol#L593-L596

1000 10^6 10^12 / 10^12 = 1000 * 10^6, exactly what we want.

[ololade97]

The question would be what does _weight[token] store?

/// @notice The token weights. Used for maintaining a non-symmetric vault asset balance.
    mapping(address => uint256) public _weight;

Based on your example, _weight[token] stores decimal points - 10^12 .

However, based on relevant lines of code in the withdrawAll function, _weight[token] doesn't store decimals points.

These are relevant lines of code in the withdrawAll function:

address[MAX_ASSETS] memory tokenIndexed;

 for (it = 0; it < MAX_ASSETS;) {
                address token = _tokenIndexing[it];
                if (token == address(0)) break;
                tokenIndexed[it] = token;
                uint256 weight = _weight[token];

for (uint256 it; it < MAX_ASSETS;) {
            address token = tokenIndexed[it];

[reednaa]

They are not storing the decimal point but the difference in decimal point.

But even that is not entirely correct. It can just as well store the difference in price. Say you have BTC and halfABTC if both had 18 decimals, you could set one to 10 and the other to 5. That would make the price of 1 BTC == 2 halfABTC.

[ololade97]

Looking at the code in the withdrawAll function, _weight[token] doesn't store the price difference.

I believe this is a bug.

[reednaa]

I don't understand. Can you provide a PoC?

[ololade97]

I will work on it.