sherlock-audit / 2024-11-telcoin-judging

0 stars 0 forks source link

whitehair0330 - In the case that `feeToken = POL`, it is highly likely that `_buyBack` will revert. #117

Open sherlock-admin3 opened 1 week ago

sherlock-admin3 commented 1 week ago

whitehair0330

High

In the case that feeToken = POL, it is highly likely that _buyBack will revert.

Summary

In a defiSwap, the process first transfers feeToken from the users' wallets to the AmirX contract, where the received feeToken is swapped for telcoin. However, if feeToken = POL, POLs from SWAPPER_ROLE are used for the swap instead of the POL received from the users' wallets. This is highly likely to revert due to a mismatch with swapData.

Root Cause

As noted in line 232 of the AmirX._buyBack() function, msg.value is swapped for telcoin. This msg.value comes from SWAPPER_ROLE, not from the users' wallets. As a result, the defiSwap is executed incorrectly and is highly likely to revert due to a mismatch with swapData.

    function _buyBack(
        ERC20 feeToken,
        address aggregator,
        address safe,
        bytes memory swapData
    ) internal {
        if (address(feeToken) == address(0)) return;
        if (address(feeToken) == POL) {
232         (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");
            }
        } else {
            // zero out approval
            feeToken.forceApprove(aggregator, 0);
            feeToken.safeIncreaseAllowance(
                aggregator,
                feeToken.balanceOf(address(this))
            );

            (bool ercSwap, ) = aggregator.call{value: 0}(swapData);
            require(ercSwap, "AmirX: token swap transaction failed");

            uint256 remainder = feeToken.balanceOf(address(this));
            if (remainder > 0) feeToken.safeTransfer(safe, remainder);
        }
    }

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

defiSwap is performed incorrectly and is highly likely to revert, breaking the core functionality of the protocol.

PoC

No response

Mitigation

Use the contract's balance instead of msg.value.

    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);
+           (bool polSwap, ) = aggregator.call{value: address(this).balance}(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");
            }
        } else {
            // zero out approval
            feeToken.forceApprove(aggregator, 0);
            feeToken.safeIncreaseAllowance(
                aggregator,
                feeToken.balanceOf(address(this))
            );

            (bool ercSwap, ) = aggregator.call{value: 0}(swapData);
            require(ercSwap, "AmirX: token swap transaction failed");

            uint256 remainder = feeToken.balanceOf(address(this));
            if (remainder > 0) feeToken.safeTransfer(safe, remainder);
        }
    }
sherlock-admin2 commented 1 week ago

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