Protocol fees collected in PairFees are lost due to accrued yield

[hats-bug-reporter[bot]]

Github username: -- Twitter username: yixxas Submission hash (on-chain): 0xae1f76b5a32d3e0351aa083da118a068f0119b47c0927f634bc829d9b6526fbc Severity: high

Description: Description\ Fenix dexV2.sol functions like the typical dex, and support stable and non stable pairs. Rebasing ERC20 tokens such as USDB and WETH are supported. We can verify this by seeing that Pair.sol inherits BlastERC20RebasingManage so that yield for these rebasing token on Blast can be set.



Protocol fees collected are collected in terms of both token0 and token1 in _update0 and _update1. These fees are sent to the PairFees contract. claimFees() can only claim up to the maximum amount as accounted for in the claimable0[] and claimable1[] array. It cannot claim any excess fees that were generated due to Blast yield. Fees due to WETH and USDB rebasing up will be lost forever.

Recommended mitigation\ PairFees should inherit BlastERC20RebasingManage as well so that it can set ERC20 yield mode to CLAIMABLE. Note that the default yield mode is AUTOMATIC.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

[BohdanHrytsak]

Thank you for the submission.

Yes, indeed, if there is a WETH/*, USDB/ pair, the commission tokens that will be sent to PairFees will work in Automatic mode and will result in part of the balance being blocked on this contract. I agree with this.

I also consider this overseverity for the following reasons:

1. No user funds will be lost, blocked, etc.
2. Security is not compromised

This problem only causes the loss of some lost protocol revenue

[yixxas]

Hi @BohdanHrytsak 



Thank you for your comments. I would still like to stand by my initial rating of this issue as a high severity one.


According to the contest guidelines,

High

- Theft or long term freezing of unclaimed yield or other assets.



This issue falls exactly under this category impact wise. It is a permanent freezing of unclaimed yield. Furthermore, this issue has an almost 100% probability of occurring. Once a pair that is USDB/ or WETH/ is deployed and traded, the fees that are sent to the PairFees will lose its accrued yield permanently. Hence, likelihood of this issue is high too. USDB/ and WETH/ are also likely to be one of the most common token pairings used. USDB is basically all of the 3 biggest USD tokens - USDT, DAI, USDC. Any of USDT, DAI, USDC deposited on L1 will be minted as USDB on L2. Impact on loss of protocol revenue is not to be understated. 

 

 

I would also like to add that holding WETH on blast is inherently more risky (slashing) than holding WETH on other chains. By missing out on any yield, protocol is taking on the risk, but none of the benefits.

[BohdanHrytsak]

Thank you for your comment.

Yes, indeed, if this happens, the transfer of USDB/WETH to PairFees will be lost, then the potential income from the amount of commissions will be lost.

For me, it's something between medium/high severity. I was going off of whether this would actually happen in the state of the protocol as it will be deployed

We can see that the default settings assume that 100% of the fees are sent to FeesVault, and PairFees will remain empty

In the future, if this configuration for WETH/, USDB/ pairs were to be changed it would lead to this issue and the loss of fees until it was noticed. Therefore, this issue is valid if the configuration is changed for these pairs, and the loss will be the unearned revenue from the % of commissions sent to FeesVault until it is detected.

Complexity of the problem: Configuration error and its change by the authorized user Probability: Guaranteed in case of a configuration change for these pairs/general, invalid for the initial state of the protocol Losses: unearned income by the protocol % of the commission, for a certain indefinite period

[yixxas]

Hi @BohdanHrytsak. Thank you for your response again.

If we look at the Pair.sol contract, in _updateFor(),

    function _updateFor(address recipient) internal {
        uint _supplied = balanceOf[recipient]; // get LP balance of `recipient`
        if (_supplied > 0) {
            uint _supplyIndex0 = supplyIndex0[recipient]; // get last adjusted index0 for recipient
            uint _supplyIndex1 = supplyIndex1[recipient];
            uint _index0 = index0; // get global index0 for accumulated fees
            uint _index1 = index1;
            supplyIndex0[recipient] = _index0; // update user current position to global position
            supplyIndex1[recipient] = _index1;
            uint _delta0 = _index0 - _supplyIndex0; // see if there is any difference that need to be accrued
            uint _delta1 = _index1 - _supplyIndex1;
            if (_delta0 > 0) {
                uint _share = (_supplied * _delta0) / 1e18; // add accrued difference for each supplied token
                claimable0[recipient] += _share;
            }   
            if (_delta1 > 0) {
                uint _share = (_supplied * _delta1) / 1e18;
                claimable1[recipient] += _share;
            }   
        } else {
            supplyIndex0[recipient] = index0; // new users are set to the default global state
            supplyIndex1[recipient] = index1;
        }   
    } 

claimable0[] and claimable1[] is updated for users who have added liquidity to the pool. This is to reward liquidity providers of the protocol. This function is called whenever an update is required, such as during minting and burning.

claimFees() allows users to claim fees for based on the value accrued in claimable0[] and claimable1[]. This amount claimed is transferred from the PairFees.sol contract.

    function claimFees() external returns (uint claimed0, uint claimed1) {
        _updateFor(msg.sender);

        claimed0 = claimable0[msg.sender];
        claimed1 = claimable1[msg.sender];

        if (claimed0 > 0 || claimed1 > 0) {
            claimable0[msg.sender] = 0; 
            claimable1[msg.sender] = 0;

            PairFees(fees).claimFeesFor(msg.sender, claimed0, claimed1);

            emit Claim(msg.sender, msg.sender, claimed0, claimed1);
        }
    }

This means that for users to claim fees, there MUST be sufficient fees sitting in the PairFees.sol contract, or users will not be able to claim even if claimable0[msg.sender] || claimable1[msg.sender] > 0. Also note that there is no way to change the deployed instance of PairFees for a Pair, as they are deployed in initialize() for each Pair.

Unless protocol intends to maintain the default setting of 100% protocol fee and not reward any users for any LP provided (which should not be the case at all, because why implement PairFees then?), then this should be a high severity issue.

[BohdanHrytsak]

@yixxas All logic like claimFees,calculation, etc. is inherited and is a standard way from Uniswap, when fees from swap operations are given to liquidity providers.

In our case, the protocol follows the ve(3,3) model and plans to give 100% of the swap fees as bribes to veFNX voters. Rewards for LP providers are provided in other ways in the form of issuing FNX/other tokens through Gauge or MerklDistribution

For us, a non-standard case is the distribution of swap fees not equal to 100% in certain specific cases

Therefore, the problem itself depends on whether we will use less than 100% for WETH/USDB pools in the future.

It's really hard to decide what severity this issue should be, but because of the set of mitigations, I'm more inclined to medium.