`Auction.sol#auctionOngoing` Switching between 1, 2 instead of true, false is more gas efficient

[code423n4]

Handle

WatchPug

Vulnerability details

The current implementation of auctionOngoing is switching between true and false.

SSTORE from false (0) to true (1) (or any non-zero value), the cost is 20000; SSTORE from 1 to 2 (or any other non-zero value), the cost is 5000.

By storing the original value once again, a refund is triggered (https://eips.ethereum.org/EIPS/eip-2200).

Since refunds are capped to a percentage of the total transaction's gas, it is best to keep them low, to increase the likelihood of the full refund coming into effect.

Therefore, switching between 1, 2 instead of false (0), true (1) will be more gas efficient.

See: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/86bd4d73896afcb35a205456e361436701823c7a/contracts/security/ReentrancyGuard.sol#L29-L33

Recommendation

Change to:

    uint256 private constant _NOT_ONGOING = 1;
    uint256 private constant _ONGOING = 2;
    uint256 public override auctionOngoing;

    function startAuction() onlyBasket public override {
        require(auctionOngoing == false, 'ongoing auction');

        auctionOngoing = _ONGOING;
        // ...
    }

    function killAuction() onlyBasket public override {
        auctionOngoing = _NOT_ONGOING;
    }

    function initialize(address basket_, address factory_) public override {
        require(address(factory) == address(0));
        require(!initialized);

        basket = IBasket(basket_);
        factory = IFactory(factory_);
        auctionOngoing = _NOT_ONGOING;
        initialized = true;
    }

    function bondForRebalance() public override {
        require(auctionOngoing == _ONGOING);
        // ...
    }

    function settleAuction(
        uint256[] memory bountyIDs,
        address[] memory inputTokens,
        uint256[] memory inputWeights,
        address[] memory outputTokens,
        uint256[] memory outputWeights
    ) public nonReentrant override {
        require(auctionOngoing == _ONGOING);
        require(hasBonded);
        require(bondTimestamp + ONE_DAY > block.timestamp);
        require(msg.sender == auctionBonder);
        require(inputTokens.length == inputWeights.length);
        require(outputTokens.length == outputWeights.length);

        for (uint256 i = 0; i < inputTokens.length; i++) {
            IERC20(inputTokens[i]).safeTransferFrom(msg.sender, address(basket), inputWeights[i]);
        }

        for (uint256 i = 0; i < outputTokens.length; i++) {
            IERC20(outputTokens[i]).safeTransferFrom(address(basket), msg.sender, outputWeights[i]);
        }

        //TODO: name a and b or further split up
        uint256 a = factory.auctionMultiplier() * basket.ibRatio();
        uint256 b = (bondBlock - auctionStart) * BASE / factory.auctionDecrement();
        uint256 newRatio = a - b;

        (address[] memory pendingTokens, uint256[] memory pendingWeights, uint256 minIbRatio) = basket.getPendingWeights();
        require(newRatio >= minIbRatio);
        IERC20 basketAsERC20 = IERC20(address(basket));

        for (uint256 i = 0; i < pendingWeights.length; i++) {
            uint256 tokensNeeded = basketAsERC20.totalSupply() * pendingWeights[i] * newRatio / BASE / BASE;
            require(IERC20(pendingTokens[i]).balanceOf(address(basket)) >= tokensNeeded);
        }

        basket.setNewWeights();
        basket.updateIBRatio(newRatio);
        auctionOngoing = _NOT_ONGOING;
        hasBonded = false;

        basketAsERC20.safeTransfer(msg.sender, bondAmount);
        withdrawBounty(bountyIDs);

        emit AuctionSettled(msg.sender);
    }

    function bondBurn() external override {
        require(auctionOngoing == _ONGOING);
        require(hasBonded);
        require(bondTimestamp + ONE_DAY <= block.timestamp);

        basket.auctionBurn(bondAmount);
        hasBonded = false;
        auctionOngoing = _NOT_ONGOING;
        basket.deleteNewIndex();

        emit BondBurned(msg.sender, auctionBonder, bondAmount);

        auctionBonder = address(0);
    }