Solidity_ATL_Team_2 - Napier pool owner can unfairly increase protocol fees on swaps to earn more revenue

[sherlock-admin]

Solidity_ATL_Team_2

medium

Napier pool owner can unfairly increase protocol fees on swaps to earn more revenue

Summary

Currently there is no limit to how often a poolOwner can update fees which can be abused to earn more fees by charging users higher swap fees than they expect.

Vulnerability Detail

The NapierPool::setFeeParameter function allows the poolOwner to set the protocolFeePercent at any point to a maximum value of 100%. The poolOwner is a trusted party but should not be able to abuse protocol settings to earn more revenue. There are no limits to how often this can be updated.

Impact

A malicious poolOwner could change the protocol swap fees unfairly for users by front-running swaps and increasing fees to higher values on unsuspecting users. An example scenario is:

• The poolOwner sets swap fees to 1% to attract users
• The poolOwner front runs all swaps and changes the swap fees to the maximum value of 100%
• After the swap the poolOwner resets protocolFeePercent to a low value to attract more users

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/NapierPool.sol#L544-L556 https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/libs/PoolMath.sol#L313

Tool used

Manual Review and Foundry

Proof of concept

 function test_protocol_owner_frontRuns_swaps_with_higher_fees() public whenMaturityNotPassed {
        // pre-condition
        vm.warp(maturity - 30 days);
        deal(address(pts[0]), alice, type(uint96).max, false); // ensure alice has enough pt
        uint256 preBaseLptSupply = tricrypto.totalSupply();
        uint256 ptInDesired = 100 * ONE_UNDERLYING;
        uint256 expectedBaseLptIssued = tricrypto.calc_token_amount([ptInDesired, 0, 0], true);

        // Pool owner sees swap about to occur and front runs updating fees to max value
        vm.startPrank(owner);
        pool.setFeeParameter("protocolFeePercent", 100);
        vm.stopPrank();

        // execute
        vm.prank(alice);
        uint256 underlyingOut = pool.swapPtForUnderlying(
            0, ptInDesired, recipient, abi.encode(CallbackInputType.SwapPtForUnderlying, SwapInput(underlying, pts[0]))
        );
        // sanity check
        uint256 protocolFee = SwapEventsLib.getProtocolFeeFromLastSwapEvent(pool);
        assertGt(protocolFee, 0, "fee should be charged");
    }

Recommendation

Introduce a delay in fee updates to ensure users receive the fees they expect.

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

[massun-onibakuchi]

we have acknowledge the issue

[cvetanovv]

As the sponsor wrote in the other report, slippage protection can prevent a malicious increase in fees.

[Darkartt]

Escalate

[sherlock-admin2]

Escalate

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.

[Robert-H-Leonard]

There is slippage protection in the swap but it does not fully protect against a malicious increase.

The PoC in this issue can be added to the test suite in v1-pool/test/unit/pool/Swap.t.sol to show this increase is possible. The default protocol fee is set to 80% and this test is setting it to 100% and performing the swap.

To test an extreme case that still passes you can decrease the default protocol fee to 5% (which is configured here and still have the fee increase.

[Czar102]

@cvetanovv can you elaborate on your stance, also given the new context?

[cvetanovv]

I agree with the escalation. This report and #98 should be Medium.

[Czar102]

Planning to apply the suggestion. Any idea why is the escalation empty? I'm not sure if I should accept it or choose this one to be accepted for this single modification of the validity.

[Robert-H-Leonard]

@Czar102 the escalation is empty because we are working on a team of 5 and he was the only team member that could raise it. My comment below his escalation is the context of the escalation

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• Darkartt: accepted

[Czar102]

Result: Medium Has duplicates

[Czar102]

@Robert-H-Leonard next time please just share the escalation contents with each other to put the recommendation in the escalation. This won't be accepted in the future.