jennifer37 - Uninitialized cache.redeemFee cause 0 redeem fee

[sherlock-admin4]

jennifer37

Medium

Uninitialized cache.redeemFee cause 0 redeem fee

Summary

cache.redeemFee is not initialized correctly, which cause LP holders don't need to pay the redeem fee. This is not expected behavior.

Vulnerability Detail

When LP holders redeem liquidity, LP holders need to pay some redeem fees. In function _executeRedeemStakeToken, the actual redeem fee is calculated and save into the variable redeemFee. The vulnerability is that contract use cache.redeemTokenAmount - cache.redeemFee to calculate the final amount that LP holders can redeem. However, cache.redeemFee is not initialised and the default value is 0.

This means that the LP holders don't need to pay the redeem fee. This is not one expected behavior. What's more, this redeem fee is charged by fee rewards. In the future, these redeem fees may be transferred out to reward contract. However, in fact, LP holders don't leave any redeem fees. This could cause the LP pool's account into a mess.

    function _executeRedeemStakeToken(
        LpPool.Props storage pool,
        Redeem.Request memory params,
        address baseToken
    ) internal returns (uint256) {
       ......
@==> actual redeem Fee.
        uint256 redeemFee = FeeQueryProcess.calcMintOrRedeemFee(cache.redeemTokenAmount, poolConfig.redeemFeeRate);
        FeeProcess.chargeMintOrRedeemFee(
            redeemFee,
            params.stakeToken,
            params.redeemToken,
            params.account,
            FeeProcess.FEE_REDEEM,
            false
        );
@==> cache.redeemFee is not initialized to redeemFee.
        VaultProcess.transferOut(
            params.stakeToken,
            params.redeemToken,
            params.receiver,
            cache.redeemTokenAmount - cache.redeemFee
        );

Impact

• LP holders don't need to pay the redeem fees.
• The redeem fees are charged by fee rewards. However, LP holders don't leave any redeem fees in the pool, which will cause the pool's account into a mess.

Code Snippet

https://github.com/sherlock-audit/2024-05-elfi-protocol/blob/main/elfi-perp-contracts/contracts/process/RedeemProcess.sol#L133-L183

Tool used

Manual Review

Recommendation

Initialized cache.redeemFee

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/0xCedar/elfi-perp-contracts/pull/20

[Hash01011122]

Escalate This is low/info severity issue. Or atleast should reduce severity of this issue.

cache.redeemFee is not initialized and the default value is 0.

Initialization of parameters aren't considered as valid High/medium issue.

Another question to @nevillehuang: Even if we consider, What is the probability of LP holders not paying redeem fees when redeem fees isn't cached and what amount Fee rewards are lost? Can you help me understand it with some mathematical model if possible.

[sherlock-admin3]

Escalate This is low/info severity issue. Or atleast should reduce severity of this issue.

cache.redeemFee is not initialized and the default value is 0.

Initialization of parameters aren't considered as valid High/medium issue.

Another question to @nevillehuang: Even if we consider, What is the probability of LP holders not paying redeem fees when redeem fees isn't cached and what amount Fee rewards are lost? Can you help me understand it with some mathematical model if possible.

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.

[goheesheng]

Escalate This is low/info severity issue. Or atleast should reduce severity of this issue.

cache.redeemFee is not initialized and the default value is 0.

Initialization of parameters aren't considered as valid High/medium issue.

Another question to @nevillehuang: Even if we consider, What is the probability of LP holders not paying redeem fees when redeem fees isn't cached and what amount Fee rewards are lost? Can you help me understand it with some mathematical model if possible.

I think that this report stating "initialization" can be misleading.

What is the probability of LP holders not paying redeem fees when redeem fees isn't cached

Every user will not be paying them.

[nevillehuang]

@Hash01011122 There is no probability required. Any and all redemptions of stake tokens executed by keeper from LPs will not pay the intended redemption fees.

[WangSecurity]

To clarify, the redemption fee cannot be set later after the contracts are deployed, correct?

[Hash01011122]

@0xELFi @0xELFi02 Would you mind responding to @WangSecurity question.

Also, can anyone help me understand what's the point of cache.redeemTokenAmount? And do redeem token inherently caches redeem fees?

[WangSecurity]

I wouldn’t say it necessary for the sponsors to answer. I can’t see the function to set the fees later, but wanted someone to clarify if I’m missing it or not.

[johnson37]

@WangSecurity , per my understanding, the cache.redeemFee should be set via below way. And use this after we set it. We should not create one new temporary variable redeemFee to record this fee.

         StakingAccount.Props storage stakingAccountProps = StakingAccount.load(params.account);
         AppPoolConfig.LpPoolConfig memory poolConfig = AppPoolConfig.getLpPoolConfig(pool.stakeToken);
-        uint256 redeemFee = FeeQueryProcess.calcMintOrRedeemFee(cache.redeemTokenAmount, poolConfig.redeemFeeRate);
+        cache.redeemFee = FeeQueryProcess.calcMintOrRedeemFee(cache.redeemTokenAmount, poolConfig.redeemFeeRate);
+        // @audit_fp is this correct here to use false, currently we don't support true.
         FeeProcess.chargeMintOrRedeemFee(
-            redeemFee,
+            cache.redeemFee,
             params.stakeToken,
             params.redeemToken,
             params.account,
             FeeProcess.FEE_REDEEM,
             false
         );
        VaultProcess.transferOut(
            params.stakeToken,
            params.redeemToken,
            params.receiver,
            cache.redeemTokenAmount - cache.redeemFee
        );

[WangSecurity]

Yep, I understand that it should be in that way, but since it's not and as I understand indeed cache.redeemFee cannot be set after the contest is deployed, I agree it's a valid finding and an issue, not a design decision.

Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: High Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• Hash01011122: rejected

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.