Denial of Service (DoS) and Gas Grief Attack in Token Redemption Process

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xa5b4b9894d4e91e80f7a5a9a31763d4a7e23170ca42a7e834d046b8f62023549 Severity: medium

Description: Description\ An attacker can potentially launch a Denial of Service (DOS) or Gas Griefing attack when a user attempts to redeem their tokens in the current redemption process.

Attack Scenario\ When a user redeems their tokens, the contract iterates over the outcomeIndexes to calculate the token balance and then transfers it back from the user to the contract. The vulnerable code section is as follows:

        for (uint256 j = 0; j < outcomeIndexes.length; j++) {
            indexSets[j] = 1 << outcomeIndexes[j];
            tokenId = getTokenId(collateralToken, parentCollectionId, conditionId, indexSets[j]);

            // first we need to unwrap the outcome tokens that will be redeemed.
            (IERC20 wrapped1155, bytes memory data) = market.wrappedOutcome(outcomeIndexes[j]);
@>            uint256 amount = wrapped1155.balanceOf(msg.sender);

            wrapped1155.transferFrom(msg.sender, address(this), amount);

Attack Steps:

1. The victim user intends to redeem their tokens and approves the necessary token allowance.
2. The victim calls the redeem function, which starts the token redemption process.
3. The attacker detects the victim's redemption call and front-runs it by sending a small amount of tokens (1 wei) to the last token in the outcomeIndexes.
4. During the victim’s transaction, the loop proceeds over all the outcomeIndexes. When it reaches the last index, the redemption logic calculates the user’s balance (now altered by the attacker’s 1 wei transfer).
5. The contract then attempts to transfer this altered balance, causing the transaction to revert.

This forces the victim’s transaction to revert, causing wasted gas and preventing the redemption process.

Impact:

• Denial of Service (DOS): The attacker can prevent the victim from redeeming their tokens by constantly front-running the transaction, making it revert.
• Gas Griefing: The victim will incur gas costs with every failed attempt to redeem, wasting funds.

Revised Code File (Optional)

Modify the redeem function to accept the exact amounts of tokens to be redeemed as input. This prevents the contract from recalculating the balance on-chain, thus avoiding manipulation by attackers.

Notes:

A note on this could be that users will approve max so there will be no problem, but I'm talking about normal users with different wallets, some wallet warn users to approve more than the balance and it's not a normal way to handle the process.

[0xmahdirostami]

note that the issue doesn't exist in conditional tokens as they use another approach(_burn doesn't check for allowance ). https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/interaction/conditional-tokens/ConditionalTokens.sol#L248-L253

but the issue exists in the _redeemPositions function, so effected functions will be: redeemToDai, redeemToBase, redeemPositions

[greenlucid]

Medium severity vulnerability description: Issues that lead to an economic loss but do not lead to significant loss of on-chain assets. Examples are:

• Attacks that make essential functionality of the contracts unusable or inaccessible (ex: make the router inoperable requiring a new router deployment or users to call functions individually to redeem, repeatedly prevent the creation of a market) without putting funds at risk.

I would say that, for this to qualify as Medium vulnerability, the users' funds should be effectively frozen in place for that amount of time. Since the interface can direct the user to do a max approve, it's an interfacing issue.

but I'm talking about normal users with different wallets, some wallet warn users to approve more than the balance

I argue the behaviour of the user wallet is also a component of interfacing, not responsibility of Seer to handle. Some wallets like MetaMask attempt to override the frontend allowance into something different, some others respect the frontend call.

and it's not a normal way to handle the process.

If the token you'll allowing is a token that only makes sense to be approved in the context of a 1-time merging or redeeming, that after the market resolves will have effectively no value, and the target that obtains the approval is audited and cannot be upgraded, there's nothing insecure about awarding a max approval

[0xmahdirostami]

@greenlucid thanks for commenting:

i would say that it will cause issues for Smart Contracts and Smart Contract Wallets that integrate with routers as well. Smart contracts will usually approve the amount, not the max as you did in some parts of your codebase.

collateralToken.approve(address(conditionalTokens), amount);

so the issue here will be Dos Smart Contracts and Smart Contract Wallets that will integrate and work with routers.

Medium severity vulnerability description: Issues that lead to an economic loss but do not lead to significant loss of on-chain assets. Examples are:

Attacks that make essential functionality of the contracts unusable or inaccessible (ex: make the router inoperable requiring a new router deployment or users to call functions individually to redeem, repeatedly prevent the creation of a market) without putting funds at risk. I would say that, for this to qualify as Medium vulnerability, the users' funds should be effectively frozen in place for that amount of time. Since the interface can direct the user to do a max approve, it's an interfacing issue.

YES, I read this. but I think this issue will be under this category as well.

Smart Contracts and Smart Contract Wallets will be DOSed by this issue. normal users who don't read code, don't need to max approve and here you ask for extra trust from them.

Gas grief: reverting the transaction in last iterate, and the user must approve again and call redeem again.

[clesaege]

So this report seems valid. We could see users not using a max approval and then this frontrunning leading to the TX to revert. It doesn't quite fit the description of medium "Attacks that make essential functionality of the contracts unusable or inaccessible", as the user can just make a larger approval, so it's an annoyance, not making "unusable or inaccessible".

As way to fix this would be to have the amount specified as an input (and it's also simpler and saves gas).

[rodiontr]

So this report seems valid. We could see users not using a max approval and then this frontrunning leading to the TX to revert. It doesn't quite fit the description of medium "Attacks that make essential functionality of the contracts unusable or inaccessible", as the user can just make a larger approval, so it's an annoyance, not making "unusable or inaccessible".

As way to fix this would be to have the amount specified as an input (and it's also simpler and saves gas).

This is not an issue as the amount that will be transferred is the same + wei that was transferred from an attacker. Frontrunning just increases the balance of the user before redemption and this balance will be redeemed. What's the problem here?

[clesaege]

@rodiontr The issue lies in that the user may have only done a partial approve, so the amount of tokens which the router would try to transfer would be higher than that.

[rodiontr]

@rodiontr The issue lies in that the user may have only done a partial approve, so the amount of tokens which the router would try to transfer would be higher than that.

sorry what do you mean by user's partial approve? The only time when user makes an approval is when he deposits/splits but the issue is about redeeming tokens. And when tokens are transferred back to the user, there is no user's approval involved. So this issue is basically nonsense. But anyway that's your that's your business and this will affect security of your protocol so I can only provide my feedback

[clesaege]

I mean not approving uint.max but the amount of tokens they currently own. In this issue, they are not transferred back, but from the user.

[clesaege]

Fixed in https://github.com/seer-pm/demo/pull/42

[0xmahdirostami]

Fixed in seer-pm#42

@clesaege Seems good, but I think it would be better to have a list of amounts. Users may have different amounts for different indexes.

In this way, user couldn't redeem all of them in one TRX.

function _redeemPositions(
        IERC20 collateralToken,
        Market market,
        uint256[] calldata outcomeIndexes,
        uint256[] calldata amounts
    ) internal {

btw, I think this issue could be rewarded more than 1 point (low severity). The issue could make any integrated contract completely unusable.

[clesaege]

@0xmahdirostami yeah that's correct, a list will allow to still do all in one TX

[clesaege]

Fixed in https://github.com/seer-pm/demo/pull/42 (asking for the amount of each outcome)

[clesaege]

Reviewing the submissions and updating that to medium.

Attacks that make essential functionality of the contracts unusable or inaccessible (ex: make the router inoperable requiring a new router deployment or users to call functions individually to redeem, repeatedly prevent the creation of a market) without putting funds at risk.

Even if a user could just do a higher approval, the front is asking to approve only the exact amount. And in case of such an attack a user may not understand why the redemption fails, making him unable to redeem until the problem is found. When discovered, this attack would have required a redeployment of a new router. The classification is indeed ambiguous as we could argue that "Redeeming with the exact amount approved is not an essential functionality" as you can redeem with a uint.max approve. But in practice, due to the way users are expected to interact with the contract, it would be blocking an essential functionality (at least for some non negligible amount of time).

The report is also well written and I could understand the issue immediately upon reading it. So I'll also add the High Quality Report bonus.

[rodiontr]

Reviewing the submissions and updating that to medium.

Attacks that make essential functionality of the contracts unusable or inaccessible (ex: make the router inoperable requiring a new router deployment or users to call functions individually to redeem, repeatedly prevent the creation of a market) without putting funds at risk.

Even if a user could just do a higher approval, the front is asking to approve only the exact amount. And in case of such an attack a user may not understand why the redemption fails, making him unable to redeem until the problem is found. When discovered, this attack would have required a redeployment of a new router. The classification is indeed ambiguous as we could argue that "Redeeming with the exact amount approved is not an essential functionality" as you can redeem with a uint.max approve. But in practice, due to the way users are expected to interact with the contract, it would be blocking an essential functionality (at least for some non negligible amount of time).

The report is also well written and I could understand the issue immediately upon reading it. So I'll also add the High Quality Report bonus.

there is no approval in redeeming process, only when users deposit tokens. Still don’t understand why you think this is an issue. When user approves to the router, router then transfer collateral to another token. But when it’s a reverse operation, router will still transfer the amount that user requested without any issue

[clesaege]

@rodiontr You need to approve the ERC20 tokens to the router so it can unwrap and redeem them for you.

[rodiontr]

@rodiontr You need to approve the ERC20 tokens to the router so it can unwrap and redeem them for you.

that's not true:

https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/MainnetRouter.sol#L52-61

 function redeemToDai(Market market, uint256[] calldata outcomeIndexes) external {
        uint256 initialBalance = sDAI.balanceOf(address(this));

        _redeemPositions(sDAI, market, outcomeIndexes);

        uint256 finalBalance = sDAI.balanceOf(address(this));

        if (finalBalance > initialBalance) {
            sDAI.redeem(finalBalance - initialBalance, msg.sender, address(this));
        }

User only approves when splitting:

https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/MainnetRouter.sol#L33

 DAI.approve(address(sDAI), amount);

[clesaege]

That's the transferFrom here which requires a previous approval.

[Tobi0x18]

@clesaege I have one question from curiosity. Usually, frontrunning is impossible for L2 as the mempool is private. Is it possible for Gnosis Chain?

[0xmahdirostami]

@Auditor0x18 Protocols usually prefer to prevent these kinds of vulnerabilities. For high-quality protocols, this could have many reasons: future decentralization (as we could see many issues related to front running accepted for other L2s as well), redeployment on other chains. ….

It is possible that this issue may not be front-running as well. Users may perform an approval at one time and then call Redeem at another time.

BTW, if there is an integrated contract, that contract may approve an exact amount, if someone sends 1 Wei token to that contract, makes that contract completely unstable.

Other contract, contract A:

token.approve(router, amount)
router.redeemPositions

router:

token.balanceOf(contract A)
token.transferFrom(contract A, address(this), balance) //@audit fail

[0xmahdirostami]

@rodiontr

the issue occurs in the following lines:

https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/Router.sol#L180-L182

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/8b591baef460523e5ca1c53712c464bcc1a1c467/contracts/token/ERC20/ERC20.sol#L151

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/8b591baef460523e5ca1c53712c464bcc1a1c467/contracts/token/ERC20/ERC20.sol#L301-L305

    function _spendAllowance(address owner, address spender, uint256 value) internal virtual {
        uint256 currentAllowance = allowance(owner, spender);
        if (currentAllowance != type(uint256).max) {
            if (currentAllowance < value) {
                revert ERC20InsufficientAllowance(spender, currentAllowance, value);

As _redeemPositions is an internal function, it will affect the current three routers and all potential upcoming routers.

[Tobi0x18]

@0xmahdirostami I knew that this vulnerability can be exploited without frontrunning. However, since the report focused on frontrunning, and attacks based on this method are typically considered invalid on other platforms like Sherlock and Code4Rena, I wanted to highlight that.

Regardless, it was a great catch, and I believe you deserve rewards for it. However, I still think the report could be improved, and it would be better if it were not solely based on frontrunning. If it were submitted to Sherlock or Code4Rena, it would likely be invalidated. Just a suggestion for reference.

[0xmahdirostami]

@Auditor0x18 I don't know why in every competition someone tries to invalidate my submission with a new GitHub account.

If it were submitted to Sherlock or Code4Rena, it would likely be invalidated.

👀i know that it's not true, actually it depends on the submission

So your reference for judging in Hatsfinance is C4 and Sherlock .

as a person who has some experience in judging, i would say that every platform has its own rules, you couldn't judge an issue on Immunefi based on Sherlock scope and vice versa.

Even in Sherlock, there is a term that said that:

Historical decisions are not considered sources of truth.

Please be sure before adding comments.

Note that in hats sponsors judge issues, so they know what is best for their protocol, and they know what issues must be mitigated,

[Tobi0x18]

@0xmahdirostami I submitted a report based on frontrunning on L2, but it was invalidated for the reasons mentioned above. I’ve experienced this before, and it was out of curiosity.

Is this possible for Hats? I'm noting the different rules across platforms since I’m new to Hats.

Additionally, I didn’t imply that you are not eligible for rewards.

Regardless, it was a great catch, and I believe you deserve rewards for it.

For my account, it’s not new and the only difference from yours is that it doesn’t have a polished introduction page.

[clesaege]

@clesaege I have one question from curiosity. Usually, frontrunning is impossible for L2 as the mempool is private. Is it possible for Gnosis Chain?

We can have private mempool on Gnosis but I don't expect all users to use it.

Also keep in mind it will also be deployed on mainnet.

[clesaege]

I do agree that the classification as medium is disputable, but since we're far from reaching the max payout anyways, I don't expect it to be.

In this contest the rules of frontrunning are as follows:

Issues about frontrunning provided that either:

• It doesn't affect the normal functioning of the system (ex: someone frontruns answering a question, but gives the same answer). Only leading to a loss of gas and/or a loss of opportunity.
• There are effective mitigations put in place (ex: someone frontruns answering a question, giving a different answer, but now the initial party can either put a higher bond or make a dispute).

For the first part, repeatedly giving one wei of token would affect the normal functioning. Now the more subjective part is "Is there effective mitigations put in place?". One could argue that you can do a higher approval. But the front is not proposing this. You could do it by manually changing your approval from your wallet (metamask for example allows that). So the classification is not obvious, but in this case we choose to classify in favor of the hunter.