060.md

Warm Red Porcupine

Medium

AmirX.sol :: defiSwap() when referralFee is 0, is vulnerable to front-running, which can cause the transaction to revert.

Summary

defiSwap() is designed to perform a token swap on a DEX. However if the referralFee is 0, an attacker could front-run the transaction by sending just 1 wei of TELECOIN to the contract, causing the transaction to revert.

Root Cause

In the final step of defiSwap(), after the swap is completed, it calls _feeDispersal() to send the fee to the referrer.

function _feeDispersal(DefiSwap memory defi) internal {
        // must buy into TEL
        if (defi.feeToken != TELCOIN)
            _buyBack(
                defi.feeToken,
                defi.aggregator,
                defi.defiSafe,
                defi.swapData
            );

        // distribute reward
        if (defi.referrer != address(0) && defi.referralFee != 0) {
            TELCOIN.forceApprove(address(defi.plugin), 0);
            TELCOIN.safeIncreaseAllowance(
                address(defi.plugin),
                defi.referralFee
            );
            require(
                defi.plugin.increaseClaimableBy(
                    defi.referrer,
                    defi.referralFee
                ),
                "AmirX: balance was not adjusted"
            );
        }
        // retain remainder
        if (TELCOIN.balanceOf(address(this)) > 0)
            TELCOIN.safeTransfer(
                defi.defiSafe,
                TELCOIN.balanceOf(address(this))
            );
    }

As you can see, the final step involves sending any remaining TELECOIN that wasn't allocated to the referrer. This becomes problematic when defi.referralFee is 0 because defi.defiSafe is 0 too.

An attacker could exploit this by front-running the transaction, sending 1 wei of TELECOIN to the contract and forcing it to attempt a transfer to address(0), which would cause the transaction to revert.

I’m assuming that when defi.referralFee is 0, defi.defiSafe is also 0, based on how the test is implemented and my understanding that transactions will be conducted with these input values. Source

Internal pre-conditions

defi.defiSafe == address(0)

External pre-conditions

None.

Attack Path

1. A user calls defiSwap() with defiSafe = address(0) and referralFee = 0. (This data is provided by the protocol; referralFee doesn’t necessarily need to be 0, but this is how it's implemented.)

2. An attacker spots the transaction in the mempool and sends 1 wei of TELECOIN to AmirX.

3. The transaction then reverts because it attempts to transfer 1 wei to address(0).

Impact

All swaps will revert.

PoC

To reproduce the issue, add the following POC to AmirX.test.ts.

it("wallet call frontrunned", async () => {
                const defiInputs = {
                    defiSafe: ZERO_ADDRESS,
                    aggregator: ZERO_ADDRESS,
                    plugin: ZERO_ADDRESS,
                    feeToken: await telcoin.getAddress(),
                    referrer: ZERO_ADDRESS,
                    referralFee: 0,
                    walletData: await wallet.getTestSelector(),
                    swapData: '0x',
                }

                await telcoin.approve(AmirX, 10);

                //attacker front run
                await telcoin.transfer(AmirX, 1);

                await expect(AmirX.defiSwap(holder, defiInputs)).to.be.reverted; 
            });

Mitigation

To resolve the issue, before sending the balance of the contract, ensure that defi.defiSafe != address(0).

function _feeDispersal(DefiSwap memory defi) internal {
        // must buy into TEL
        if (defi.feeToken != TELCOIN)
            _buyBack(
                defi.feeToken,
                defi.aggregator,
                defi.defiSafe,
                defi.swapData
            );

        // distribute reward
        if (defi.referrer != address(0) && defi.referralFee != 0) {
            TELCOIN.forceApprove(address(defi.plugin), 0);
            TELCOIN.safeIncreaseAllowance(
                address(defi.plugin),
                defi.referralFee
            );
            require(
                defi.plugin.increaseClaimableBy(
                    defi.referrer,
                    defi.referralFee
                ),
                "AmirX: balance was not adjusted"
            );
        }
        // retain remainder
-        if (TELCOIN.balanceOf(address(this)) > 0)
+        if (TELCOIN.balanceOf(address(this)) > 0 && defi.defiSafe != address(0))
            TELCOIN.safeTransfer(
                defi.defiSafe,
                TELCOIN.balanceOf(address(this))
            );
    }