sherlock-audit / 2024-11-telcoin-judging

0 stars 0 forks source link

elvin.a.block - `AmirX._buyBack` will sweep unrelated POL tokens along with buyback remainders as POL transfer sends entire contract balance #158

Open sherlock-admin3 opened 1 week ago

sherlock-admin3 commented 1 week ago

elvin.a.block

Medium

AmirX._buyBack will sweep unrelated POL tokens along with buyback remainders as POL transfer sends entire contract balance

Summary

The missing balance tracking in AmirX._buyBack function will cause unintended loss of accumulated POL tokens for the AmirX contract as the function sends the entire contract's POL balance rather than just the remainder from the current buyback operation

Root Cause

In AmirX.sol:235-238 the AmirX._buyBack function sends the entire contract's POL balance via address(this).balance when handling POL remainders after a buyback operation, instead of tracking POL balance difference and sending only the msg.value remainder from the current operation. This leads to sweeping all POL tokens in the contract, including those unrelated to the current buyback.

Internal pre-conditions

  1. Contract needs to have accumulated POL balance from sources other than current buyback operation (e.g. direct transfers or other operations)
  2. AmirX.swap() or AmirX.defiSwap() needs to be called with a DefiSwap struct containing POL as feeToken
  3. The aggregator swap operation needs to return some POL remaining after the swap

External pre-conditions

No specific external protocol conditions required

Attack Path

  1. User or another contract directly sends POL to AmirX contract, building up POL balance unrelated to buybacks
  2. Authorized SWAPPER_ROLE calls AmirX.swap() or AmirX.defiSwap() with valid parameters and POL as feeToken
  3. The operation reaches AmirX._buyBack() function
  4. After performing POL to TELCOIN swap via aggregator, some POL remains
  5. AmirX._buyBack() executes safe.call{value: address(this).balance}(""), sending ALL contract's POL balance to safe address, including the pre-existing POL that was unrelated to this buyback

Impact

The AmirX contract suffers loss of accumulated POL tokens unrelated to the current buyback operation. While these funds can be recovered via AmirX.rescueCrypto(), it requires additional gas costs and administrative overhead from SUPPORT_ROLE holders to monitor and recover the tokens. The attacker does not directly gain from this but could use it to force unnecessary operations and gas consumption from the protocol administrators.

PoC

Mitigation

The contract should track only the remaining portion of msg.value after the swap operation, since this represents the actual remainder from the current buyback. Here's the correct fix:

function _buyBack(
    ERC20 feeToken,
    address aggregator,
    address safe,
    bytes memory swapData
) internal {
    if (address(feeToken) == address(0)) return;
    if (address(feeToken) == POL) {
        // Store initial balance
        uint256 initialBalance = address(this).balance;

        // Perform the swap
        (bool polSwap, ) = aggregator.call{value: msg.value}(swapData);
        require(polSwap, "AmirX: POL swap transaction failed");

        // Calculate how much POL was actually used by aggregator
        uint256 balanceDiff = initialBalance - address(this).balance;

        // Calculate remaining portion of msg.value that wasn't used
        uint256 remainingPOL = msg.value - balanceDiff;

        // Send only the remaining portion of msg.value
        if (remainingPOL > 0) {
            (bool success, ) = safe.call{value: remainingPOL}("");
            require(success, "AmirX: POL send transaction failed");
        }
    } else {
        // ... rest of function for non-POL tokens
    }
}