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

[sherlock-admin2]

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

takarez commented:

valid because { valid as watson demostrated how this implementation will lead to an inflation attack of the ERC4626 but its medium due to the possibility of it is very low and that front-tun in arbitrum is very unlikely }

[detectiveking123]

Escalate

I am not completely sure about the judgement here and am therefore escalating to get @Czar102 's opinion on how this should be judged.

I believe that #56 and #21 should be treated as different issues than this one. I am not even sure if this issue and other duplicates are valid, as they rely on the front-running on Arbitrum assumption, which has not been explicitly confirmed to be valid or invalid on Sherlock.

Please take a look at the thread on #56 to better understand the differences. But the TLDR is:

1. This issue requires Arbitrum frontrunning to work, the other one in detectiveking - FundingRateArbitrage suffers same share-inflation issues as ERC4626 vaults #56 doesn't
2. The one in detectiveking - FundingRateArbitrage suffers same share-inflation issues as ERC4626 vaults #56 takes advantage of a separate rounding error as well to fully drain funds inside the contract

[sherlock-admin]

Escalate

I am not completely sure about the judgement here and am therefore escalating to get @Czar102 's opinion on how this should be judged.

I believe that #56 and #21 should be treated as different issues than this one. I am not even sure if this issue and other duplicates are valid, as they rely on the front-running on Arbitrum assumption, which has not been explicitly confirmed to be valid or invalid on Sherlock.

Please take a look at the thread on #56 to better understand the differences. But the TLDR is:

1. This issue requires Arbitrum frontrunning to work, the other one in detectiveking - FundingRateArbitrage suffers same share-inflation issues as ERC4626 vaults #56 doesn't
2. The one in detectiveking - FundingRateArbitrage suffers same share-inflation issues as ERC4626 vaults #56 takes advantage of a separate rounding error as well to fully drain funds inside the contract

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[JoscelynFarr]

Fixed PR: JOJOexchange/smart-contract-EVM@b3cf3d6

[nevillehuang]

@detectiveking123

• I think both this issue and your issue implies that the user needs to be the first depositor, especially the scenario highlighted. This does not explicitly requires a front-run, given a meticulous user can make their own EV calculations.

This issue:

Assume the user deposits 1 share then donates 100,000e6 USDC. The exchange ratio is now 100,000e18 which causes issues during deposits.

Your issue:

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.

• I think one of the watsons in the discord channel highlighted a valid design of the protocol to mitigate this issue, where withdrawals must be explicitly requested. Because of this, this could possibly be medium severity.

[giraffe0x]

Disagree that the request/permit design prevents this.

As described in code comments for requestWithdraw(): "The main purpose of this function is to capture the interest and avoid the DOS attacks". It is unlikely that withdraw requests are individually scrutinized and manually permitted by owner but automatically executed by bots in batches. Even if the owner does monitor each request, it would be tricky to spot dishonest deposits/withdrawals.

It is better to implement native contract defence a classic ERC4626 attack. Should be kept as a high finding.

[nevillehuang]

@giraffe0x Again you are speculating on off-chain mechanisms. While it is a valid concern, the focus should be on contract level code logic, and sherlocks assumption is that admin will make the right decisions when permitting withdrawal requests.

Also, here is the most recent example of where first depositor inflation is rated as medium:

sherlock-audit/2023-12-dodo-gsp-judging#55

[detectiveking123]

@nevillehuang

"I think both this issue and your issue implies that the user needs to be the first depositor, especially the scenario highlighted. This does not explicitly requires a front-run, given a meticulous user can make their own EV calculations."

How would you run this attack without front-running? Share inflation attacks explicitly require front-running

[nevillehuang]

@detectiveking123 I agree that the only possible reason for this issue to be valid is if

• It so happens that a depositor is the first depositor, and he made his own EV calculations and realized so based on the shares obtained and current exchange ratios (slim chance of happening given front-running is a non-issue on arbitrum, but not impossible)
• If the attack does not explicitly require a first depositor inflation attack (to my knowledge not possible), which I believe none of the original issues showed a scenario/explanation it is so (Even Ignite - The first depositor of FundingRateArbitrage contract can drain all users' USDC #21 and detectiveking - FundingRateArbitrage contract can be drained due to rounding error #57 is highlighting a first depositor scenario)

If both of the above scenario does not apply, all of the issues and its duplicates should be low severity.

[IAm0x52]

Fix looks good. Adds a virtual offset which prevents this issue.

[Evert0x]

Front-running isn't necessary as the attacker can deposit 1 wei + donate and just wait for someone to make a deposit under 100,000e6.

But this way the attack is still risky to execute as the attacker will lose the donated USDC in case he isn't the first depositor.

However, this can be mitigated if the attacker created a contract the does the 1 wei deposit + donate action in a single transaction BUT revert in case it isn't the first deposit in the protocol.

Planning to reject escalation and keep issue state as is.

[detectiveking123]

@Evert0x not sure that makes sense, if you just wait for someone then they should just not deposit (it's a user mistake to deposit, they should be informed that they'll retrieve no shares back in the UI).

[Czar102]

After a discussion with @Evert0x, planning to make it a Medium severity issue – frontend can display information for users not to fall victim to this exploit by displaying a number of output shares.
Even though frontrunning a tx can't be done easily (there is no mempool), one can obtain information about a transaction being submitted in another way. Since this puts severe constraints on this being exploitable, planning to consider it a medium severity issue.

[detectiveking123]

@Czar102 My issue that has been duplicated with this (#57) drains the entire contract (clearly a high) and requires no front-running. The purpose of the escalation was primarily to request deduplication.

[detectiveking123]

@IAm0x52 So you agree the post audit code you have audited and agreed solves the share inflation issue is vulnerable to the rounding attack?

[IAm0x52]

No it's not vulnerable to the rounding attack due to withdrawal minimums and deposit fees.

[detectiveking123]

@IAm0x52 Okay, my previous exploit assumed that withdrawal minimum and deposit fees were zero. Let's do a concrete example involving them. We will assume that withdrawalMinimum = 1 USD and deposit fee = 0.1%, like you mentioned in your earlier comment.

Note: This is on the post-audit, fixed codebase.

1. Deposit $1000. 1000 shares will be minted, for a share price of $1. I will pay $1 in deposit fees for this.
2. Let a bunch of other people deposit.
3. Deposit $2 in. I will receive two shares for this. I will pay 0.2 cents in deposit fees for this.
4. Withdraw and send in $1 JUSD + 1 wei of JUSD, which will be rounded to two shares. Profit: around a dollar.
5. Repeat steps 3 and 4 for as long as you want.

Unless someone has challenges regarding the validity of this example, I will rest my case here and leave it to the head of judging. The fact that the issue exists and drains the contract in a version of the codebase that the LW has agreed fixes share inflation proves that this issue is distinct and valid.

[IAm0x52]

1. Deposit $1000. 1000 shares will be minted, for a share price of $1. I will pay $1 in deposit fees for this.

This is again mistaken. Using the changed code our initial index would be:

1e18 * (1 + 0) / (0 + 1e3) = 1e15

This would mean that the following is minted:

1000e6 * 1e18 / 1e15 = 1,000,000e6

After deposit our index is now:

1e18 * (1 + 1000e6) / (1,000,000e6 + 1e3) = ~1e15

Now lets finish the example:

2. Let a bunch of other people deposit
3. Deposit $2. You will receive 2000e6 shares and pay $0.002 in deposit fees
4. Withdraw and send in 1,000,001 wei of JUSD. Which will be rounded to 1,000,001,000 shares. Profit: $0 - Loss: $0.002 (deposit fees)
5. Repeat steps 3 and 4 until you run out of money.

[detectiveking123]

@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.

[IAm0x52]

The problem you run into is that now when another person deposits, they will mint with an index of:

1e18 * (1 + 2e9) / (1000 + 1e3) = 5e5 * 1e18 (half the original index)

Which will cause the attacker to immediately lose 1000 USDC because supply has the offset enabled. This is why we use the virtual offset because it will prevent all gains by counteracting the inflation. The more people that deposit the more it depresses the exchange rate (and the more the attacker loses) so the users who deposited right away will be able to withdraw and profit from the attacker and now he has lost all his money.

This is the exact purpose of the virtual offset and the reason it is implemented to break inflation attacks. The more they inflate the more they lose to others' deposits, causing a vicious cycle that causes massive loss to the attacker.

[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:
#57 (comment)

[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.