Gas Optimization: Unnecessary nonReentrant on sweepFees

[code423n4]

Handle

0xngndev

Vulnerability details

Concept and Migitation

Each nonReentrant modifier adds around 0.04kb of contract size. In the case of sweepFees, the modifier is not needed as there's only one unsafe external call: feeRecipient.transfer(address(this).balance). This happens at the end of the function, meaning all other executions have already completed, and it also transfers (makes an external call) to a governance-established address, which should be safe.

[Shadowfiend]

safeTransferFrom is also called on arbitrary input (the tokens passed by the caller), so we disagree that the modifier is completely unnecessary here.

[0xean]

Agree with sponsor, the added costs for increased security seems like a good tradeoff and the external calls are not limited to the last line as stated by the warden.

For ref:

    /// @notice Sweeps accrued ETH and ERC20 swap fees to the pre-established
    ///         fee recipient address.
    /// @dev Fees are tracked based on the contract's balances, rather than
    ///      using any additional bookkeeping. If there are bugs in swap
    ///      accounting, this function could jeopardize funds.
    /// @param tokens An array of ERC20 contracts to withdraw token fees
    function sweepFees(
        address[] calldata tokens
    ) external nonReentrant {
        require(
            feeRecipient != address(0),
            "Swap::withdrawAccruedFees: feeRecipient is not initialized"
        );
        for (uint8 i = 0; i<tokens.length; i++) {
            uint256 balance = IERC20(tokens[i]).balanceOf(address(this));
            if (balance > 0) {
                IERC20(tokens[i]).safeTransfer(feeRecipient, balance);
                emit FeesSwept(tokens[i], balance, feeRecipient);
            }
        }
        feeRecipient.transfer(address(this).balance);
        emit FeesSwept(address(0), address(this).balance, feeRecipient);
    }