Naresh - Faulty Fee Validation in `SuperPool::requestFeeUpdate()` Function Leads to Update Lockout

[sherlock-admin3]

Naresh

Medium

Faulty Fee Validation in SuperPool::requestFeeUpdate() Function Leads to Update Lockout

Summary

The SuperPool::requestFeeUpdate() function is responsible for proposing new fee updates in the SuperPool contract, has a validation issue. The function incorrectly validates the current state variable fee instead of the new _fee parameter. This flawed logic causes the function to revert when the current fee exceeds 1e18, regardless of the _fee value. As a result, if the fee is ever set to a value greater than 1e18, no further fee updates can be proposed, leading to potential disruptions in the contract’s operations.

Vulnerability Detail

The SuperPool::requestFeeUpdate() is used to propose a new fee update for the SuperPool. the current implementation of the requestFeeUpdate() function only checks the state variable fee and not the new parameter _fee. This check only considers the current state variable fee, not the _fee parameter. This means if the state variable fee is greater than 1e18, the function will revert regardless of the value of _fee.

    function requestFeeUpdate(uint256 _fee) external onlyOwner {
@>     if (fee > 1e18) revert SuperPool_FeeTooHigh();
        pendingFeeUpdate = PendingFeeUpdate({ fee: _fee, validAfter: block.timestamp + TIMELOCK_DURATION });
        emit SuperPoolFeeUpdateRequested(_fee);
    }

Since the requestFeeUpdate() function only considers the current state variable fee, once it’s set to a value greater than 1e18, no new updates can be proposed. This means the contract will never allow a fee update to be requested, potentially breaking functionality that depends on the ability to update the fee.

POC

Extended from SuperPool.t.sol ```solidity function testCannot_Change_Fee() public { uint256 fee = 0.01 ether; SuperPool superpool = new SuperPool(address(pool), address(asset1), feeTo, fee, 1_000_000 ether, "test", "test"); // Update the fee to greater than 1e18 superpool.requestFeeUpdate(1e19); vm.warp(block.timestamp + superpool.TIMELOCK_DURATION() + 2); superpool.acceptFeeUpdate(); // Reverts once it is set to a value greater than 1e18, and no new updates can be proposed vm.expectRevert(SuperPool.SuperPool_FeeTooHigh.selector); superpool.requestFeeUpdate(0.01 ether); } ``` Run the following command to execute the POC: `forge test --match-test testCannot_Change_Fee`

Impact

The inability to update the fee due to the incorrect validation in requestFeeUpdate() could lead to a breakdown in the contract’s intended operations, making the contract non-functional or less adaptable to future needs.

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/SuperPool.sol#L366-L370

Tool used

Manual Review

Recommendation

Correct the validation in the requestFeeUpdate() function to check the _fee parameter:

    function requestFeeUpdate(uint256 _fee) external onlyOwner {
--      if (fee > 1e18) revert SuperPool_FeeTooHigh();
++      if (_fee > 1e18) revert SuperPool_FeeTooHigh();
        pendingFeeUpdate = PendingFeeUpdate({ fee: _fee, validAfter: block.timestamp + TIMELOCK_DURATION });
        emit SuperPoolFeeUpdateRequested(_fee);
    }

[S3v3ru5]

🤦‍♂️ for not reporting this issue. Considering the fee is an admin input, the check only serves as a sanity check. I did not expect this to be consider valid medium

[0x3b33]

Escalate fee is an admin input. Thus we can't consider this valid by Sherlock rules. It's low at best.

[sherlock-admin3]

Escalate fee is an admin input. Thus we can't consider this valid by Sherlock rules. It's low at best.

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.

[serial-coder]

Escalate fee is an admin input. Thus we can't consider this valid by Sherlock rules. It's low at best.

Disagree. This is a VALID issue.

The vulnerability stemmed from an incorrect implementation that is unaligned with the protocol's spec (the correct implementation must use _fee (function's param) instead of the fee (state variable) for the validation check). More specifically, the vulnerability allows a SuperPool owner to mistakenly set the fee parameter to be greater than 1e18 (over the protocol's spec).

According to the sponsor's comment on the public channel:

SuperPool depositors should trust the owner when it comens to param changes and configs but if there's a mechanism flaw it should be taken under consideration as an issue

I would like to emphasize the sponsor's quote, "if there's a mechanism flaw it should be taken under consideration as an issue".

Further, the sponsor also confirmed this issue with Sponsor Confirmed and Will Fix tags. Please refer to my issue (#510) for more details, including two coded PoCs.

[cvetanovv]

I agree with Escalation that this Issue is invalid.

Any consequences would come from the wrong admin input. And the admin is TRUSTED.

Even the quote from the Discord channel proves it.

SuperPool depositors should trust the owner when it comens to param changes and configs but if there's a mechanism flaw it should be taken under consideration as an issue

The admin is trusted to call the function with the correct input.

Planning to accept the escalation and invalidate the issue.

[NareshETH]

The quote from the Discord channel clearly states that if there's a mechanism flaw, it should be taken under consideration as an issue. Additionally, the sponsor confirmed this issue with the Sponsor Confirmed tag. This is not about the admin being trusted to call the function with the correct input; the issue is that the code itself contains a mechanism flaw.

[serial-coder]

Hi @cvetanovv,

With all due respect, I disagree with your reason.

The vulnerability stemmed from an incorrect implementation that is unaligned with the protocol's spec (the correct implementation must use _fee (function's param) instead of the fee (state variable) for the validation check).

More specifically, the vulnerability allows a SuperPool owner to mistakenly set the fee parameter to be greater than 1e18 (over the protocol's spec).

[cvetanovv]

Just because the admin put a "Confirmed" label doesn't mean the issue is a valid Medium. This means it is a valid low severity. In Sherlock Low severity issues are invalid issues.

The quote from the Discord channel clearly states that if there's a mechanism flaw, it should be taken under consideration as an issue.

Yes, indeed, there is an issue, and it is Low severity because we have no impact that the parameter is wrong. Admin can still set the correct values. He is trusted to make it correct.

If the admin sets wrong values by mistake, it is an invalid issue.

My decision to accept the escalation remains.

[serial-coder]

@cvetanovv

You said:

Yes, indeed, there is an issue, and it is Low severity because we have no impact that the parameter is wrong. Admin can still set the correct values. He is trusted to make it correct.

You said: "Admin can still set the correct values".

    function requestFeeUpdate(uint256 _fee) external onlyOwner {
        //@audit -- An incorrect validation on the state variable, fee, instead of the inputted parameter _fee.
@>      if (fee > 1e18) revert SuperPool_FeeTooHigh();

        pendingFeeUpdate = PendingFeeUpdate({ fee: _fee, validAfter: block.timestamp + TIMELOCK_DURATION });
        emit SuperPoolFeeUpdateRequested(_fee);
    }

You're wrong. Please look at the above code, if you mistakenly set the fee (state variable) > 1e18, you will no longer be able to correct it because the function will always revert the tx.

You said: "we have no impact that the parameter is wrong".

Here are my coded PoCs (issue #510). PoC #2 shows that the fee can be set to reach the maximum limit of the uint256 (i.e., type(uint256).max), causing the SuperPool's functions to be DoS'ed. Consequently, all deposited assets will get stuck in the pool.

There are two test functions. Execute the commands:

• forge t --nmt "Fork|invariant" --mt testPoCSetSuperPoolFeeOver1e18
• forge t --nmt "Fork|invariant" --mt testPoCSetSuperPoolFeeOver1e18_DoS

PoC #1 shows that the SuperPool owner can mistakenly set the fee over 1e18 (100%), i.e., to 50000000%.

PoC #2 shows that the fee can be set to reach the maximum limit of the uint256 (i.e., type(uint256).max), causing the SuperPool's functions to be DoS'ed. Consequently, all deposited assets will get stuck in the pool.

function testPoCSetSuperPoolFeeOver1e18() public { // PoC #1
    // Before the fee update (1%)
    assertEq(superPool.fee(), 0.01 ether); // 1%

    // Request for the fee update (1% -> 50000000%)
    vm.startPrank(poolOwner);
    superPool.requestFeeUpdate(500000 ether); // 50000000%

    vm.warp(24 hours + 1 seconds);
    superPool.acceptFeeUpdate();
    vm.stopPrank();

    // After the fee update (50000000%)
    // Incorrect implementation, not aligned with the spec
    assertEq(superPool.fee(), 500000 ether); // 50000000%
}

function testPoCSetSuperPoolFeeOver1e18_DoS() public { // PoC #2
    // --- Setup ---
    vm.prank(poolOwner);
    superPool.addPool(linearRatePool, 1 ether);

    // --- User deposits 1 ether of asset1 to the SuperPool ---
    vm.startPrank(user);
    asset1.mint(user, 1 ether);
    asset1.approve(address(superPool), 1 ether);

    uint256 expectedShares = superPool.previewDeposit(1 ether);
    uint256 shares = superPool.deposit(1 ether, user);
    assertEq(shares, expectedShares);

    assertEq(asset1.balanceOf(address(pool)), 1 ether);

    // --- User redeems 10% of deposited amount (0.1 ether) ---
    // Assets before withdrawal: 1 ether
    assertEq(superPool.convertToAssets(superPool.balanceOf(user)), 1 ether);

    superPool.redeem(shares / 10, user, user);
    vm.stopPrank();

    // Assets after withdrawal: 0.9 ether
    assertEq(superPool.convertToAssets(superPool.balanceOf(user)), 0.9 ether);

    // --- SuperPool owner requests for the fee update (1% -> MAX%) ---
    // Before the fee update (1%)
    assertEq(superPool.fee(), 0.01 ether); // 1%

    vm.startPrank(poolOwner);
    superPool.requestFeeUpdate(type(uint256).max); // MAX%

    vm.warp(24 hours + 1 seconds);
    superPool.acceptFeeUpdate();

    // After the fee update (MAX%)
    // Incorrect implementation, not aligned with the spec
    assertEq(superPool.fee(), type(uint256).max); // MAX%

    // --- Donate 1 wei to trigger the DoS ---
    asset1.mint(poolOwner, 1 wei);
    asset1.transfer(address(superPool), 1 wei);
    vm.stopPrank();

    // --- User tries to redeem another 10% of the deposited amount (0.1 ether) ---
    // Remaining deposited amount == 0.9 ether
    vm.startPrank(user);
    vm.expectRevert(); // Tx will revert with "Arithmetic Underflow error"
    superPool.redeem(shares / 10, user, user);
}

[serial-coder]

Hi @ruvaag,

Per the above comments, could we have another opinion from the sponsor?

Thanks for your time, sir.

[cvetanovv]

@serial-coder If the admin is TRUSTED, he won't make mistakes.

You wrote in the last comment:

You're wrong. Please look at the above code, if you mistakenly set the fee (state variable) > 1e18, you will no longer be able to correct it because the function will always revert the tx.

The admin is TRUSTED and will not make mistakes. This means he will always set the correct values. These are the rules.

In the case where he mistakenly sets wrong values, we don't look at it at all because he is TRUSTED.

My decision to accept the escalation and invalidate the issue remains.

[NareshETH]

While the sponsor acknowledged that the admin is TRUSTED, they also stated that "if there's a mechanism flaw, it should be taken under consideration as an issue."

This vulnerability arises from an implementation that is not aligned with the protocol's specification. Specifically, the correct implementation should validate the _fee parameter (the function input) instead of the fee state variable. Given this, it should be considered a mechanism flaw.

[serial-coder]

@cvetanovv,

You only see what you want it to be. I gave you the proofs (PoCs), but you invalidated them even if they proved that your assumptions were wrong.

[cvetanovv]

@serial-coder @NareshETH I don't want to argue with you. It's not my job to argue, it's my job to make the best decision.

To calm you down that my decision is not intentional to invalidate it, I will leave this issue for someone else to look at later.

[WangSecurity]

@cvetanovv is correct above that this issue should be invalid based on the rules (note: the issue not wrong, but based on the rules this issue should be unrewarded and isn't enough to be medium). As I understand, there's no information about this fee in the README (IIUC it's not Default Interest Fee). Hence, the following rule applies:

(External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path. If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

This issue can happen only if the fee is > 1e18, while any value <= 1e18 will not cause any issues. Hence, the admin is trusted to use values <=1e18 to not cause any issues.

*Note: even if it's Default Interest Fee, the max Default Interest Fee is 1e17 (as stated in the README). Hence, this issue won't happen as 1e17 is < 1e18.

The decision remains, accept the escalation and invalidate the issue.

[NareshETH]

@WangSecurity

According to the sponsor's comment on the public channel:

"if there's a mechanism flaw, it should be taken under consideration as an issue."

The core issue here is not about the admin using trusted values but rather a flaw in the contract's logic itself. The correct implementation should validate the _fee parameter (the function input) rather than the fee state variable. This is a mechanism flaw, and regardless of the trust placed in the admin, this issue could lead to an unintended lockout, which should be considered for its potential impact.

Per the above comments, could we have another opinion from the sponsor? @ruvaag

[WangSecurity]

The discord messages don't stand above the rules and the README. Based on the rules and the README, it's invalid. Planning to reject the escalation and leave the issue as it is.

[iamnmt]

@WangSecurity

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

Shouldn't the escalation be accepted, and the issue be invalidated?

[WangSecurity]

Yes, excuse me, planning to accept the escalation and invalidate the issue.

[Evert0x]

Result: Invalid Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0x3b33: accepted

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/sentimentxyz/protocol-v2/pull/315