Variation of inflation attack in `absorber` makes it possible for an attacker to start griefing users

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/absorber.cairo#L667-L689

Vulnerability details

Impact

It is possible to start griefing users who provide their yin to the absorber, making them lose all of their funds in exchange of 0 shares (a variation of the vanilla first-depositor front-running attack).

Proof of Concept

Like any other ERC4626 vault, the absorber implements a deposit function called provide that, in a nutshell, lets users deposit their yin and receive shares of the underlying vault. To prevent the vanilla front-running attack, the first provider must deposit a value higher or equal than INITIAL_SHARES, so that he will receive the difference:

absorber, function convert_shares

        fn convert_to_shares(self: @ContractState, yin_amt: Wad, round_up: bool) -> (Wad, Wad) {
            let total_shares: Wad = self.total_shares.read();

            if INITIAL_SHARES > total_shares.val {
                // By subtracting the initial shares from the first provider's shares, we ensure that
                // there is a non-removable amount of shares. This subtraction also prevents a user
                // from providing an amount less than the minimum shares.
                assert(yin_amt.val >= INITIAL_SHARES, 'ABS: provision < minimum');
                return ((yin_amt.val - INITIAL_SHARES).into(), yin_amt);
            }

            ...

        }

and if we go to the definition of INITIAL_SHARES:

    const INITIAL_SHARES: u128 = 1000; // 10 ** 3 (Wad);

we see this amount is negligible taking into account the decimals of the yin token (18 as seen here). If we add the fact that convert_to_shares can return 0 shares and provide does not revert if that happens, the next situation arises:

1. Bob, our average user, calls provide with his own yin to participate in absorptions by being credited a certain number of shares. Note that he is the first depositor and lets say the amount provided by Bob is X
2. Alice, our attacker, sees this, front-runs Bob and provides exactly INITIAL_SHARES, so that she receives 0 shares AND self.total_shares = self.yin_erc20().balance_of(absorber) = INITIAL_SHARES
3. After that, Alice transfers directly to the absorber X * INITIAL_SHARES, so that self.yin_erc20().balance_of(absorber) = INITIAL_SHARES + X * INITIAL_SHARES whilst self.total_shares remains unchanged
4. Bob transaction gets executed but receives 0 shares in exchange due to:
   1. self.convert_to_shares nor self.provide do not revert in that situation
   2. self.convert_to_shares does not round up if called from self.provide, as the flag is false:

            // Calculate number of shares to issue to provider and to add to total for current epoch
            // The two values deviate only when it is the first provision of an epoch and
            // total shares is below the minimum initial shares.
            let (new_provision_shares, issued_shares) = self.convert_to_shares(amount, false);

3. The expected amount Bob would receive would be X * total_shares / absorber_balance, and if we plug in the above we have X * INITIAL_SHARES / (INITIAL_SHARES + X * INITIAL_SHARES) = 0 due to rounding down in integer division if denominator > numerator

            let absorber: ContractAddress = get_contract_address();
            let yin_balance: u256 = self.yin_erc20().balance_of(absorber);

            let (computed_shares, r, _) = u256_safe_divmod(
                yin_amt.into() * total_shares.into(), yin_balance.try_into().expect('Division by zero')
            );
            let computed_shares: u128 = computed_shares.try_into().unwrap();
            if round_up && r.is_non_zero() {
                return ((computed_shares + 1).into(), (computed_shares + 1).into());
            }
            (computed_shares.into(), computed_shares.into())

For reference, check the last example of this post as well as this discussion.

Recommended Mitigation Steps

There are many solution available and you can see some of them here, although I would stick to minting the initial shares when deploying the absorber, so that there are no first-depositor attacks nor issues that. Moreover, if convert_to_shares returns 0 shares, I would revert to prevent users from providing their yin for nothing.

Assessed type

Math

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-pre-sort]

bytes032 marked the issue as duplicate of #196

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The Warden details a potential attack that revolves around front-running as well as the sacrifice of assets.

First, front-running attacks are impossible in Starknet due to the usage of a single FIFO sequencer. The #179 scenario does not rely on a front-run but rather a back-run attack, introducing a transaction after add_yang is called which is valid.

Secondly, the would-be attacker would have to sacrifice their assets to impact the assets of another, resulting in a net negative for every party involved. Such attacks are not profitable and thus reduce the impact of the exhibit.

Coupling the above facts, this submission does not constitute an HM vulnerability and is better suited as a QA (L) issue.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Overinflated severity