hunter_w3b - Incorrect Fee Handling Due to `msg.value` Mismanagement

[sherlock-admin2]

hunter_w3b

High

Incorrect Fee Handling Due to msg.value Mismanagement

Summary

A design flaw in how the contract handles msg.value will cause incorrect fee handling for POL transactions, as the contract relies on msg.value to pay the fee before the swap is completed, which is not appropriate. This mismatch can lead to overcharging or undercharging the swapper, resulting in unexpected fund loss for users.

Root Cause

The choice to use msg.value to handle fees for POL transactions is a mistake, as the actual fee is determined after the swap completes. This will cause the contract to either overcharge or undercharge users, depending on whether msg.value is set incorrectly.

• In AmirX.sol : The reliance on msg.value to handle fee payments before the swap happens, despite the actual fee being determined after the swap, introduces the logical bug.

    function _buyBack(
        ERC20 feeToken,
        address aggregator,
        address safe,
        bytes memory swapData
    ) internal {
        if (address(feeToken) == address(0)) return;
        if (address(feeToken) == POL) {
            (bool polSwap, ) = aggregator.call{value: msg.value}(swapData);
            require(polSwap, "AmirX: POL swap transaction failed");

            if (address(this).balance > 0) {
                (bool success, ) = safe.call{value: address(this).balance}("");
                require(success, "AmirX: POL send transaction failed");
            }
        } 

https://github.com/sherlock-audit/2024-11-telcoin/blob/main/telcoin-audit/contracts/swap/AmirX.sol#L232

Internal pre-conditions

1. The user must call the swap function with POL as the fee token.
2. The user must set the msg.value correctly, assuming it will match the fee expected by the contract (which will not happen).
3. The contract must attempt to handle fees using msg.value before the swap completes.

External pre-conditions

No response

Attack Path

1. A user initiates a swap with POL as the fee token, assuming the msg.value they send will cover the fee.
2. The contract attempts to use msg.value to pay for the fee before the swap is executed.
3. After the swap, the contract determines the actual fee, which could differ from the msg.value initially sent.
4. If msg.value is set incorrectly, the contract either:
   • Overcharges the user (if msg.value is more than required), or
   • Undercharges the user (if msg.value is less than required).
5. This results in the contract either holding more funds than needed (loss of funds for the user) or failing to collect the correct fee.

Impact

The user suffers an approximate loss of POL due to incorrect fee collection. The contract may either:

• Overcharge the user, leading to fund loss for the user, or
• Undercharge the user, failing to collect the full fee and causing operational issues for the protocol.

PoC

No response

Mitigation

• Use Contract Balance for POL Fees: Instead of relying on msg.value for fee payments, the contract should use its POL balance after the swap completes to handle the fee payment, ensuring accuracy.
• Remove msg.value Dependency: Modify the contract to handle POL fees using the contract’s balance, consistent with how ERC20 fees are handled, to avoid overcharging or undercharging users.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/telcoin/telcoin-audit/pull/59