xiaoming90 - Front-running swap TX and update the fee rate

[sherlock-admin2]

xiaoming90

medium

Front-running swap TX and update the fee rate

Summary

Admin could front-running a swap TX, update the fee rate, and charge additional fees that the users did not expect or were not aware of, leading to a loss of assets for the victims.

Vulnerability Detail

Per the contest's README page, it stated that the admin/owner is "RESTRICTED". Thus, any finding showing that the owner/admin can steal a user's funds, cause loss of funds or harm to the users, or cause the user's fund to be struck is valid in this audit contest.

Q: Is the admin/owner of the protocol/contracts TRUSTED or RESTRICTED?

RESTRICTED

The admin/owner of the protocol has the ability to set the fee rate for the AMM's swap operation.

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

File: NapierPool.sol
543:     /// @param value value of the parameter
544:     function setFeeParameter(bytes32 paramName, uint256 value) external {
545:         if (factory.owner() != msg.sender) revert Errors.PoolOnlyOwner();
546: 
547:         if (paramName == "lnFeeRateRoot") {
548:             if (value > MAX_LN_FEE_RATE_ROOT) revert Errors.LnFeeRateRootTooHigh();
549:             lnFeeRateRoot = uint80(value); // unsafe cast here is Okay because we checked the value is less than MAX_LN_FEE_RATE_ROOT
550:         } else if (paramName == "protocolFeePercent") {
551:             if (value > MAX_PROTOCOL_FEE_PERCENT) revert Errors.ProtocolFeePercentTooHigh();
552:             protocolFeePercent = uint8(value); // unsafe cast here is Okay
553:         } else {
554:             revert Errors.PoolInvalidParamName();
555:         }
556:     }

A malicious admin can perform the following steps to steal a user's funds:

1. The current fee rate for the swap is 1%.
2. A user submits a swap TX and expects to pay a 1% swap fee.
3. A malicious admin could front-run the swap TX, and execute to setFeeParameter to increase the fee rate to the maximum allowable value (e.g. 5%)
4. When the user's swap is executed, it will be charged a fee rate of 5% instead of 1%.
5. The admin/protocol wallet gains an extra 4%, which the user is not aware of.

Impact

Loss of assets for the victim.

Code Snippet

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

Tool used

Manual Review

Recommendation

Implement a timelock for making any change to the pool parameter.

Duplicate of #51

[sherlock-admin]

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

takarez commented:

invalid

[massun-onibakuchi]

Due to slippage protection, users are protected in case of unreasonably high fee rate.

[nevillehuang]

Escalate

I believe this issue is still a valid medium severity finding given restricted protocol admins as the admin can simply bypass slippage by front-running and charging the maximum possible increment in fees to bypass slippage and extract value to cause a loss of funds out of users.

[sherlock-admin2]

Escalate

I believe this issue is still a valid medium severity finding given restricted protocol admins as the admin can simply bypass slippage by front-running and charging the maximum possible increment in fees to bypass slippage and extract value to cause a loss of funds out of users.

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.

[massun-onibakuchi]

Are swap fees adjusted under the such restrictions unreasonably high?

[xiaoming9090]

Agree with Nevi. This should be a Medium as the admin is restricted in the Contest's README.

When Bob specifies a slippage of 100 ETH, it only indicates that he is okay with a trade slippage due to on-chain market conditions that could cost him up to 100 ETH.

If a trade costs Bob only 80 ETH and the admin steals 19 ETH, the total trading cost would be 99 ETH, below the pre-defined slippage of 100 ETH. The transaction will go through without revert. However, this does not mean that Bob is okay with malicious activities by the admin that could cost him up to 100 ETH.

These are two different topics. Thus, we cannot use the trading slippage feature to protect users against this attack. The correct mitigation for such a risk is to implement a timelock for making any change to the pool parameter.

[cvetanovv]

I agree with @nevillehuang and @xiaoming9090 comments. This report and #51 should be Medium.

[Czar102]

Planning to apply. The escalation should have been created on the main issue, but I'm not sure whether that escalation will be accepted, so the planned escalation result is TBD.

[Czar102]

Result: Medium Duplicate of #51

Indeed, but this point should have been brought up on the main issue. Hence rejecting the escalation.

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: rejected