Collected fees can be used by anyone to top-up

Summary

Anyone can use collected fees by the ExchangeProxy contract to top-up by providing arbitrary call data to the swap contract.

Vulnerability Detail

The ExchangeProxy contract collects swap fees and keeps them in escrow for later withdrawal by the yield distributor. However, an attacker is able to provide arbitrary _convertData in the HardenedTopupProxy.CardTopupPermit function. This parameter is used and passed on to the ExchangeProxy.executeSwapDirect function without validation. Then it is used to call the executorAddress to do the token swap. Hence, it's possible to instruct the swap contract to swap the collected fees from the ExchangeProxy contract instead of only swapping the tokens provided by the user (the spending allowance is set to the maximum allowance before in line 156)

Impact

An attacker can use the residual token balances (i.e. collected fees) from the ExchangeProxy contract as the amount for the top-up.

Code Snippet

To demonstrate this issue, use the provided test case can use fees from exchange proxy for topup in https://gist.github.com/berndartmueller/5cfa9d784a32ecba92eb6abaf4d464d9. Copy the test file into test/ExploitTopup.test.js and run truffle test. It demonstrates how an attacker can use 10 DAI to receive a top-up worth 900e6 USDC (collected fees).

ExchangeProxy.sol#L174

function executeSwapDirect(
    address _beneficiary,
    address _tokenFrom,
    address _tokenTo,
    uint256 _amount,
    uint256 _exchangeFee,
    bytes memory _data
) public payable override returns (uint256) {
    require(msg.sender == transferProxyAddress, "transfer proxy only");

    // extract values from bytes array provided
    address executorAddress;
    address spenderAddress;
    uint256 ethValue;

    bytes memory callData = ByteUtil.slice(_data, 72, _data.length - 72);
    assembly {
        executorAddress := mload(add(_data, add(0x14, 0)))
        spenderAddress := mload(add(_data, add(0x14, 0x14)))
        ethValue := mload(add(_data, add(0x20, 0x28)))
    }

    // allow spender to transfer tokens from this contract
    if (_tokenFrom != ETH_TOKEN_ADDRESS && spenderAddress != address(0)) {
        require(trustedRegistryContract.isWhitelisted(spenderAddress), "allowance to non-trusted");
        resetAllowanceIfNeeded(IERC20(_tokenFrom), spenderAddress, _amount);
    }

    // remember the actual balance of target token (fees could reside on this contract balance)
    uint256 balanceBefore = 0;
    if (_tokenTo != ETH_TOKEN_ADDRESS) {
        balanceBefore = IERC20(_tokenTo).balanceOf(address(this));
    } else {
        balanceBefore = address(this).balance;
    }

    // regardless of stated amount, the ETH value passed to exchange call must be provided to the contract
    require(msg.value >= ethValue, "insufficient ETH provided");

    // don't allow to call non-trusted addresses
    require(trustedRegistryContract.isWhitelisted(executorAddress), "call to non-trusted");

    // ensure no state passed, no reentrancy, etc.
    (bool success, ) = executorAddress.call{value: ethValue}(callData); // @audit-info `callData` can contain any arbitrary high swap amount
    require(success, "SWAP_CALL_FAILED");

    // always rely only on actual amount received regardless of called parameters
    uint256 amountReceived = 0;
    if (_tokenTo != ETH_TOKEN_ADDRESS) {
        amountReceived = IERC20(_tokenTo).balanceOf(address(this));
    } else {
        amountReceived = address(this).balance;
    }
    amountReceived = amountReceived.sub(balanceBefore);

    require(amountReceived > 0, "zero amount received");

    // process exchange fee if present (in deposit we get pool tokens, so process fees after swap, here we take fees in source token)
    // fees are left on this contract address and are harvested by yield distributor
    //uint256 feeAmount = amountReceived.mul(_exchangeFee).div(1e18);
    amountReceived = amountReceived.sub(
        amountReceived.mul(_exchangeFee).div(1e18)
    ); // this is return value that should reflect actual result of swap (for deposit, etc.)

    if (_tokenTo != ETH_TOKEN_ADDRESS) {
        //send received tokens to beneficiary directly
        IERC20(_tokenTo).safeTransfer(_beneficiary, amountReceived);
    } else {
        //send received eth to beneficiary directly
        payable(_beneficiary).sendValue(amountReceived);
        // payable(_beneficiary).transfer(amountReceived);
        // should work for external wallets (currently is the case)
        // but wont work for some other smart contracts due to gas stipend limit
    }

    emit ExecuteSwap(_beneficiary, _tokenFrom, _tokenTo, _amount, amountReceived);

    // amount received is used to check minimal amount condition set by calling app from topup proxy contract
    return amountReceived;
}

Tool Used

Manual Review

Recommendation

Consider validating the spent amount of the ERC-20 token _tokenFrom to equal the desired swap _amount in the ExchangeProxy.executeSwapDirect function. This prevents swapping more than the provided _amount function parameter.

Duplicate of #112

sherlock-audit / 2022-10-mover-judging

berndartmueller - Collected fees can be used by anyone to top-up #38