Unused ERC20 tokens are not refunded

[code423n4]

Handle

WatchPug

Vulnerability details

Based on the context and comments in the code, we assume that it's possible that there will be some leftover sell tokens, not only when users are selling unwrapped ETH but also for ERC20 tokens.

However, in the current implementation, only refunded ETH is returned (L158).

Because of this, the leftover tkoens may be left in the contract unintentionally.

https://github.com/code-423n4/2021-10-tally/blob/c585c214edb58486e0564cb53d87e4831959c08b/contracts/swap/Swap.sol#L153-L181

if (boughtERC20Amount > 0) {
    // take the swap fee from the ERC20 proceeds and return the rest
    uint256 toTransfer = SWAP_FEE_DIVISOR.sub(swapFee).mul(boughtERC20Amount).div(SWAP_FEE_DIVISOR);
    IERC20(zrxBuyTokenAddress).safeTransfer(msg.sender, toTransfer);
    // return any refunded ETH
    payable(msg.sender).transfer(boughtETHAmount);

    emit SwappedTokens(
        zrxSellTokenAddress,
        zrxBuyTokenAddress,
        amountToSell,
        boughtERC20Amount,
        boughtERC20Amount.sub(toTransfer)
    );
} else {

    // take the swap fee from the ETH proceeds and return the rest. Note
    // that if any 0x protocol fee is refunded in ETH, it also suffers
    // the swap fee tax
    uint256 toTransfer = SWAP_FEE_DIVISOR.sub(swapFee).mul(boughtETHAmount).div(SWAP_FEE_DIVISOR);
    payable(msg.sender).transfer(toTransfer);
    emit SwappedTokens(
        zrxSellTokenAddress,
        zrxBuyTokenAddress,
        amountToSell,
        boughtETHAmount,
        boughtETHAmount.sub(toTransfer)
    );
}

[Shadowfiend]

I believe the 0x API does in fact guarantee that we won't have any sell tokens left over, particularly since we intend to use RFQ-T for these quotes, but if not we will fix this... And we may make the change regardless to future-proof.

[0xean]

Downgrading to sev 2

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

As I believe this to be a "leak value" scenario.