MohammedRizwan - Permit functions in Union contracts can be affected by DOS

[sherlock-admin3]

MohammedRizwan

Medium

Permit functions in Union contracts can be affected by DOS

Summary

Permit functions in Union contracts can be affected by DOS

Vulnerability Detail

The following inscope Union contracts supports ERC20 permit functionality by which users could spend the tokens by signing an approval off-chain.

1. In UDai.repayBorrowWithPermit(), , after the permit call is successful there is a call to _repayBorrowFresh()

    function repayBorrowWithPermit(
        address borrower,
        uint256 amount,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external whenNotPaused {
        IDai erc20Token = IDai(underlying);
@>        erc20Token.permit(msg.sender, address(this), nonce, expiry, true, v, r, s);

        if (!accrueInterest()) revert AccrueInterestFailed();
        uint256 interest = calculatingInterest(borrower);
        _repayBorrowFresh(msg.sender, borrower, decimalScaling(amount, underlyingDecimal), interest);
    }

2. In UErc20.repayBorrowWithERC20Permit(), , after the permit call is successful there is a call to _repayBorrowFresh()

    function repayBorrowWithERC20Permit(
        address borrower,
        uint256 amount,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external whenNotPaused {
        IERC20Permit erc20Token = IERC20Permit(underlying);
@>        erc20Token.permit(msg.sender, address(this), amount, deadline, v, r, s);

        if (!accrueInterest()) revert AccrueInterestFailed();
        uint256 interest = calculatingInterest(borrower);
        _repayBorrowFresh(msg.sender, borrower, decimalScaling(amount, underlyingDecimal), interest);
    }

3. In UserManager.registerMemberWithPermit(), , after the permit call is successful there is a call to registerMember()

    function registerMemberWithPermit(
        address newMember,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external whenNotPaused {
@>        IUnionToken(unionToken).permit(msg.sender, address(this), value, deadline, v, r, s);
        registerMember(newMember);
    }

4. UserManagerDAI.stakeWithPermit(), , after the permit call is successful there is a call to stake()

    function stakeWithPermit(
        uint256 amount,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external whenNotPaused {
        IDai erc20Token = IDai(stakingToken);
@>        erc20Token.permit(msg.sender, address(this), nonce, expiry, true, v, r, s);

        stake(amount.toUint96());
    }

5. In UserManagerERC20.stakeWithERC20Permit(), , after the permit call is successful there is a call to stake()

    function stakeWithERC20Permit(
        uint256 amount,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external whenNotPaused {
        IERC20Permit erc20Token = IERC20Permit(stakingToken);
        erc20Token.permit(msg.sender, address(this), amount, deadline, v, r, s);

        stake(amount.toUint96());
    }

The issue is that while the transactions for either of above permit functions is in mempool, anyone could extract the signature parameters from the call to front-run the transaction with direct permit call.

This issue is originally submitted by Trust security aka Trust to various on chain protocols and the issue is confirmed by reputed protocols like Open Zeppelin, AAVE, The Graph, Uniswap-V2

To understand the issue in detail, Please refer below link:

link: https://www.trust-security.xyz/post/permission-denied

An attacker can extract the signature by observing the mempool, front-run the victim with a direct permit, and revert the function call for the user. "In the case that there is no fallback code path the DOS is long-term (there are bypasses through flashbots in some chains, but that's a really bad scenario to resort to)." as stated by Trust Security.

Since, the protocol would be deployed on any EVM compatible chain so Ethereum mainnet has mempool with others chain too. This issue would indeed increase the approval for the user if the front-run got successful. But as the permit has already been used, the call to either of above permit functions will revert making whole transaction revert. Thus making the victim not able to make successful call to either of above permit functions to carry out borrow repay or stake or member registration.

Consider a normal scenario,

1. Bob wants to repay his loan with permit so he calls UErc20.repayBorrowWithERC20Permit() function.

2. Alice observes the transactions in mempool and extract the signature parameters from the call to front-run the transaction with direct permit call. Alice transaction got successful due to high gas fee paid by her to minor by front running the Bob's transaction.

3. This action by Alice would indeed increase the approval for the Bob since the front-run got successful.

4. But as the permit is already been used by Alice so the call to UErc20.repayBorrowWithERC20Permit() will revert making whole transaction revert.

5. Now, Bob will not able to make successful call to UErc20.repayBorrowWithERC20Permit() function to pay his loan by using ERC20 permit(). This is due to griefing attack by Alice. She keep repeating such attack as the intent is to grief the protocol users.

Impact

Users will not be able to use the permit functions for important functions like UDai.repayBorrowWithPermit(), UErc20.repayBorrowWithERC20Permit(), UserManager.registerMemberWithPermit(), UserManagerDAI.stakeWithPermit() and UserManagerERC20.stakeWithERC20Permit() so these function would be practically unusable and users functionality would be affected due to above described issue

Code Snippet

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/market/UDai.sol#L19

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/market/UErc20.sol#L17

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/user/UserManager.sol#L711

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/user/UserManagerDAI.sol#L29

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/user/UserManagerERC20.sol#L27

Tool used

Manual Review

Recommendation

Wrap the permit calls in a try catch block in above functions using permit().

[c-plus-plus-equals-c-plus-one]

In a similar scenario this would usually be exlcuded due to "There are no intended integrations/use cases of removing liquidity with permit that makes it time sensitive (uniswapv2 has been using this same functionality for ages). The user can just call the regular remove liquidity function and they'll be good. It's difficult to consider this more than a low." (sherlock-audit/2024-02-jala-swap-judging#177 (comment))

[twicek]

Escalate
If I understand well, if someone try to use the signature of another user by monitoring the mempool they will have their call to revert at this line:

        erc20Token.permit(msg.sender, address(this), amount, deadline, v, r, s);

Since they are not the signer of the permit signature:
ERC20Permit.sol#L44-L67

    function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual {
        if (block.timestamp > deadline) {
            revert ERC2612ExpiredSignature(deadline);
        }


        bytes32 structHash = keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, _useNonce(owner), deadline));


        bytes32 hash = _hashTypedDataV4(structHash);


        address signer = ECDSA.recover(hash, v, r, s);
        if (signer != owner) {
            revert ERC2612InvalidSigner(signer, owner);
        }


        _approve(owner, spender, value);
    }```

[sherlock-admin3]

Escalate
If I understand well, if someone try to use the signature of another user by monitoring the mempool they will have their call to revert at this line:

        erc20Token.permit(msg.sender, address(this), amount, deadline, v, r, s);

Since they are not the signer of the permit signature:
ERC20Permit.sol#L44-L67

    function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual {
        if (block.timestamp > deadline) {
            revert ERC2612ExpiredSignature(deadline);
        }


        bytes32 structHash = keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, _useNonce(owner), deadline));


        bytes32 hash = _hashTypedDataV4(structHash);


        address signer = ECDSA.recover(hash, v, r, s);
        if (signer != owner) {
            revert ERC2612InvalidSigner(signer, owner);
        }


        _approve(owner, spender, value);
    }```

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.

[c-plus-plus-equals-c-plus-one]

Uniswap V2 has been using the same functions and approach for years:

    function removeLiquidityWithPermit(
        address tokenA,
        address tokenB,
        uint liquidity,
        uint amountAMin,
        uint amountBMin,
        address to,
        uint deadline,
        bool approveMax, uint8 v, bytes32 r, bytes32 s
    ) external virtual override returns (uint amountA, uint amountB) {
        address pair = UniswapV2Library.pairFor(factory, tokenA, tokenB);
        uint value = approveMax ? uint(-1) : liquidity;
        IUniswapV2Pair(pair).permit(msg.sender, address(this), value, deadline, v, r, s);
        (amountA, amountB) = removeLiquidity(tokenA, tokenB, liquidity, amountAMin, amountBMin, to, deadline);
    }

I think it should be Low at the very maximum. But the sponsor can make the final decision of course.

[c-plus-plus-equals-c-plus-one]

Just for reference, one of the UniV2 functions:
https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router02.sol#L156-L170

[NGK02]

Even if this was a DOS it would be at most a low as, according to the Sherlock docs, for a DOS to be a medium it must either affect the availability of a time-sensitive function or cause a locking of funds for more than a week. Which isn't the case here.
https://docs.sherlock.xyz/audits/judging/judging#iii.-sherlocks-standards

[c-plus-plus-equals-c-plus-one]

@NGK02 is absolutely right, in my opinion! This issue should either be a Low one or be closed if the sponsor decides not to fix that at all.

[MatinR1]

Why do you consider this a low-severity issue?
I agree that staking or registering might not seem important, but what about repaying a borrow?
The borrow repayment process clearly calculates the interest based on the time delta of the last interest accruing and the current timestamp. Any significant time delay happening here, due to DOS, increases the interest.
Repayment is essential in these protocols. This bug can lead to a temporary loss of funds, which is typically categorized as high or medium severity in most security protocols.

[mystery0x]

This finding should deserve a medium severity given the reasonings of MatinR1. Additionally, the readme indicates the protocol's desire to override the 7 days functional inaccessibility to 12 hours.

[NGK02]

@MatinR1 I consider it a low because the DOS will only last for a few minutes at the absolute most since the affected party can immediately just call the regular repayBorrow function. This will accrue an insignificant amount of interest seeing as the borrow rate is set to 0.005% per 12 seconds. Thus in my mind this DOS can't be considered to be affecting a time-sensitive function.

[twicek]

I don't think that my escalation was read but @MatinR1 comments that did not escalate was? This is weird.
In my escalation I show why there is no DoS at all @mystery0x . If I'm wrong, please elaborate.

[oluwanisola]

This is a reference to permit issue tackled in Lido and why/how the team had to mitigate it lidofinance/core#803 (comment)

It also highlights why this form of griefing is an issue that needs to be fixed.

Good to be here guys and big kudos to everyone working together to secure the codebase.

[MatinR1]

Hey all!

First of all, the issue of the borrow repayment using a permit is of medium severity as it is related to grieving. Second, the escalation reason stands for calling other users, while here the msg.sender can call which is not an escalation reason and indeed it is the architecture of the systems such as uniswap. This system equals to approving the contract on behalf of the caller to do something (e.g. staking or repaying). Here the issue is one can break this system and he/she must manually do these task. The time passing here is the main issue. (temporary loss of funds)

Also as an answer to this sentence:

uniswapv2 has been using this same functionality for ages

I want to add that UniswapV2 had rewarded this issue, though it was out of scope, and accepted the continuation of this architecture. It does not mean that this is not an issue. (reference)

Finally, after testing the issue, I've noticed that the uDai and UserManagerDai contracts wrongly implement the permit() function:

        erc20Token.permit(msg.sender, address(this), nonce, expiry, true, v, r, s);

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/market/UDai.sol#L19

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/user/UserManagerDAI.sol#L29

This passes the nonce instead of amount to the permit function. The nonce is internally counted and incremented inside the ERC20Permit contract and by doing so, it would always fail. This is a real DOS guys!

It should be changed to:

-        erc20Token.permit(msg.sender, address(this), nonce, expiry, true, v, r, s);
+        erc20Token.permit(msg.sender, address(this), amount, expiry, true, v, r, s);

@WangSecurity I managed to contact the dev team, but didn't get any response from them.

[dmitriia]

As far as I understand this is invalid precisely as escalation described, as in all the examples owner = msg.sender is included in the signature and thus step (3) This action by Alice would indeed increase the approval for the Bob cannot happen as Alice will increase approval for herself, msg.sender, not Bob.

So the original Trust EIP-2612 permission denied isn't applicable, there the prerequisite being Note that by design, the token ignores the msg.sender of the permit() call, which is not met here.

[dmitriia]

@MatinR1 Please check DAI permit signature.

[WangSecurity]

I've hidden several comments because they were just repeating the information already expressed previously.

I agree with the escalation and the last two comments, the report is incorrect and this issue will not occur on the union, because call to permit uses msg.sender.

Planning to accept the escalation and invalidate the report.

[MD-YashShah1923]

I've hidden several comments because they were just repeating the information already expressed previously.

I agree with the escalation and the last two comments, the report is incorrect and this issue will not occur on the union, because call to permit uses msg.sender.

Planning to accept the escalation and invalidate the report.

Let me give reasons why this should be considered a Medium Severity Bug
1 . As per your comments this issue would not occur in union as call to permit uses msg.sender which is false. The attacker can simply frontrun repayBorrowWithPermit() transaction when it is in the mempool, an attacker can take this signature, call the permit() function on the token(ERC20) themselves. Since this is a valid signature, the token accepts it and increases the nonce.This makes the spender's repayBorrowWithPermit() transaction fail whenever it gets mined due to increase of nonce.

Why do you consider this a low-severity issue? I agree that staking or registering might not seem important, but what about repaying a borrow? The borrow repayment process clearly calculates the interest based on the time delta of the last interest accruing and the current timestamp. Any significant time delay happening here, due to DOS, increases the interest. Repayment is essential in these protocols. This bug can lead to a temporary loss of funds, which is typically categorized as high or medium severity in most security protocols.

2. As mentioned by @MatinR1 in above comments regarding delay in repaying a borrow which causes borrower to pay more interest than normal which leads to temporary loss of funds.

@WangSecurity I can provide POC as well if you think this is not valid then.
There is a misinterpretation regarding bug due to wrong/false comments put by other participants.
I hope you re-evaluate the finding.

[dmitriia]

@MD-YashShah1923 Here msg.sender is the owner and signature includes it, so the attacker can't reuse what they observe. _useNonce(owner) is per owner too, so attacker can only increase their nonce.

[MD-YashShah1923]

@dmitriia The attacker would simply call permit() on ERC20 token using the parameters of victim's repayBorrowWithPermit().
erc20Token.permit(msg.sender, address(this), amount, deadline, v, r, s);
The above function parameters would be taken from victim's repayBorrowWithPermit().
So here, there is no case of msg.sender is the owner and the attacker can't reuse it. I hope you got the point.

[twicek]

@MD-YashShah1923
The ability to call permit directly on the token is a good argument.
However, the user will never be stuck without being able to repay, since it can just call the other repay function. repayBorrowWithPermit is just a convenience function that allows users to save gas.

Regarding the interest rate loss, even if you stretch the reality and have an account with a lot of debt, it will only be less than $1 for the time it takes to call the other function, let's say 10 minutes. So probably less than the additional amount of gas needed to call approve and gas issues are invalid.

I will let @WangSecurity decide, but given that it's possible to call permit on the token directly and increment the nonce which can lead to very small loss due to interests ticking, I would say it's on a grey area.

[MD-YashShah1923]

@twicek You are telling solution of it that use repayBorrow() only instead of repayBorrowWithPermit() but ain't talking about the bug in the repayBorrowWithPermit function.
I don't know why are you comparing this with gas issue , this issue is temporary loss of funds as borrower have to pay more interest than normal. A bug is a bug even if there is alternative function for it.

[WangSecurity]

@MD-YashShah1923 would be very interested in your POC and can you estimate the loss (in %) here if the user repays the loan in the next block (or in the next 2-3 blocks)?

[c-plus-plus-equals-c-plus-one]

As I have mentioned in the above comments, this leads to no loss for the users. They don't even need to generate a new signature, they can just for instance call repay directly, as the attacker will do them a favor by spending gas and approving the contract to use the user's funds.

So this takes away all the rationale for the attacker to perform that kind of an "attack".

This can at maximum lead to a "DoS" of a particular signature, which is not longer than a few minutes.

As per the Sherlock's rules, the medium-severity-worth DoS for it to be valid should lead to time-sensitive loses, which isn't the case for this issue. At maximum a dust amounts can be lost.

@MD-YashShah1923 @WangSecurity But anyways the end "victim" user will even profit from this "attack", because he won't need to call the more gas-consuming repayBorrowWithPermit function, as the approval would be aleady front-run by the attacker, and the user will only need to call the cheaper repayBorrow function.

P.S. A quasi account abstraction paymaster option, isn't it? 🙂

[WangSecurity]

As I have mentioned in the above comments, this leads to no loss for the users. They don't even need to generate a new signature, they can just for instance call repay directly, as the attacker will do them a favor by spending gas and approving the contract to use the user's funds.

Yep, you're correct here, but since there are other functions without the permit functionality, I don't think we need to invalidate the issues with the permit by default.

This can at maximum lead to a "DoS" of a particular signature, which is not longer than a few minutes.
As per the Sherlock's rules, the medium-severity-worth DoS for it to be valid should lead to time-sensitive loses, which isn't the case for this issue. At maximum a dust amounts can be lost.

Yep, what I was trying to understand is how much funds could be lost if we were forced to call the repay function a bit later, maybe it's larger than dust.

Since no answer was provided to my previous comment, planning to accept the escalation and invalidate the issue.

[MD-YashShah1923]

As I have mentioned in the above comments, this leads to no loss for the users. They don't even need to generate a new signature, they can just for instance call repay directly, as the attacker will do them a favor by spending gas and approving the contract to use the user's funds.

Yep, you're correct here, but since there are other functions without the permit functionality, I don't think we need to invalidate the issues with the permit by default.

This can at maximum lead to a "DoS" of a particular signature, which is not longer than a few minutes.
As per the Sherlock's rules, the medium-severity-worth DoS for it to be valid should lead to time-sensitive loses, which isn't the case for this issue. At maximum a dust amounts can be lost.

Yep, what I was trying to understand is how much funds could be lost if we were forced to call the repay function a bit later, maybe it's larger than dust.

Since no answer was provided to my previous comment, planning to accept the escalation and invalidate the issue.

Sorry @WangSecurity for replying late as was busy with other task, would provide POC till EOD.

[WangSecurity]

Since still nothing is provided, the decision remains the same and will proceed with accepting the escalation and invalidating the issue tomorrow.

[MD-YashShah1923]

There is minimum loss while repaying the borrow.
Attacker can still grief user using the steps given in the report.
But according to rules grieving attack isn't considered medium severity.
So this should be invalidated. My bad
@WangSecurity

[WangSecurity]

Result:
Invalid
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• twicek: accepted