Ocean Protocol H2O System and Action - Ocean Protocol

In the contracts GebProxyActions.sol, GebProxyAuctionActions.sol, GebProxyIncentivesActions.sol, GebProxyLeverageActions.sol, GebProxySaviourActions.sol, the return values of the external transfer calls are not checked. It should be noted that token does not revert in case of failure and return false. If one of these tokens is used, a deposit would not revert if the transfer fails, and an attacker could deposit tokens for free.

Code Location

GebProxyActions.sol

    function coinJoin_join(address apt, address safeHandler, uint wad) public {
        // Gets COIN from the user's wallet
        CoinJoinLike(apt).systemCoin().transferFrom(msg.sender, address(this), wad);

        _coinJoin_join(apt, safeHandler, wad);
    }

GebProxyAuctionActions.sol

    function claimProxyFunds(address tokenAddress) public {
        DSTokenLike token = DSTokenLike(tokenAddress);
        token.transfer(msg.sender, token.balanceOf(address(this)));
    }

GebProxyAuctionActions.sol

    function settleAuction(address auctionHouse_, uint auctionId) public {
        SurplusAuctionHouseLike auctionHouse = SurplusAuctionHouseLike(auctionHouse_);
        DSTokenLike stakedToken = DSTokenLike(auctionHouse.stakedToken());

        // Settle auction
        auctionHouse.settleAuction(auctionId);

        // Sends the staked tokens to the msg.sender
        stakedToken.transfer(msg.sender, stakedToken.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function exitAndRemoveLiquidity(address coinJoin, address incentives, address uniswapRouter, uint[2] calldata minTokenAmounts) external returns (uint amountA, uint amountB) {
        GebIncentivesLike incentivesContract = GebIncentivesLike(incentives);
        DSTokenLike rewardToken = DSTokenLike(incentivesContract.rewardsToken());
        DSTokenLike lpToken = DSTokenLike(incentivesContract.stakingToken());
        incentivesContract.exit();
        rewardToken.transfer(msg.sender, rewardToken.balanceOf(address(this)));
        return _removeLiquidityUniswap(uniswapRouter, address(CoinJoinLike(coinJoin).systemCoin()), lpToken.balanceOf(address(this)), msg.sender, minTokenAmounts);
    }

GebProxyIncentivesActions.sol

    function exitMine(address incentives) external {
        GebIncentivesLike incentivesContract = GebIncentivesLike(incentives);
        DSTokenLike rewardToken = DSTokenLike(incentivesContract.rewardsToken());
        DSTokenLike lpToken = DSTokenLike(incentivesContract.stakingToken());
        incentivesContract.exit();
        rewardToken.transfer(msg.sender, rewardToken.balanceOf(address(this)));
        lpToken.transfer(msg.sender, lpToken.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function exitRemoveLiquidityRepayDebt(address manager, address coinJoin, uint safe, address incentives, address uniswapRouter, uint[2] calldata minTokenAmounts) external {

        GebIncentivesLike incentivesContract = GebIncentivesLike(incentives);
        DSTokenLike rewardToken = DSTokenLike(incentivesContract.rewardsToken());
        DSTokenLike lpToken = DSTokenLike(incentivesContract.stakingToken());
        DSTokenLike systemCoin = DSTokenLike(CoinJoinLike(coinJoin).systemCoin());
        incentivesContract.exit();
        rewardToken.transfer(msg.sender, rewardToken.balanceOf(address(this)));

        _removeLiquidityUniswap(uniswapRouter, address(systemCoin), lpToken.balanceOf(address(this)), address(this), minTokenAmounts);

        _repayDebt(manager, coinJoin, safe, systemCoin.balanceOf(address(this)), false);
        msg.sender.call{value: address(this).balance}("");
    }

GebProxyIncentivesActions.sol

    function generateDebtAndProvideLiquidityStake(
        address manager,
        address taxCollector,
        address coinJoin,
        address uniswapRouter,
        address incentives,
        uint safe,
        uint wad,
        uint[2] calldata minTokenAmounts
    ) external payable {
        DSTokenLike systemCoin = DSTokenLike(CoinJoinLike(coinJoin).systemCoin());
        _generateDebt(manager, taxCollector, coinJoin, safe, wad, address(this));
        _provideLiquidityUniswap(coinJoin, uniswapRouter, wad, msg.value, address(this), minTokenAmounts);
        _stakeInMine(incentives);

        // sending back any leftover tokens/eth, necessary to manage change from providing liquidity
        msg.sender.call{value: address(this).balance}("");
        systemCoin.transfer(msg.sender, systemCoin.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function generateDebtAndProvideLiquidityUniswap(
        address manager,
        address taxCollector,
        address coinJoin,
        address uniswapRouter,
        uint safe,
        uint wad,
        uint[2] calldata minTokenAmounts
    ) external payable {
        DSTokenLike systemCoin = DSTokenLike(CoinJoinLike(coinJoin).systemCoin());
        _generateDebt(manager, taxCollector, coinJoin, safe, wad, address(this));

        _provideLiquidityUniswap(coinJoin, uniswapRouter, wad, msg.value, msg.sender, minTokenAmounts);

        // sending back any leftover tokens/eth, necessary to manage change from providing liquidity
        msg.sender.call{value: address(this).balance}("");
        systemCoin.transfer(msg.sender, systemCoin.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function getRewards(address incentives) public {
        GebIncentivesLike incentivesContract = GebIncentivesLike(incentives);
        DSTokenLike rewardToken = DSTokenLike(incentivesContract.rewardsToken());
        incentivesContract.getReward();
        rewardToken.transfer(msg.sender, rewardToken.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function lockETHGenerateDebtProvideLiquidityStake(
        address manager,
        address taxCollector,
        address ethJoin,
        address coinJoin,
        address uniswapRouter,
        address incentives,
        uint safe,
        uint deltaWad,
        uint liquidityWad,
        uint[2] memory minTokenAmounts
    ) public payable {
        DSTokenLike systemCoin = DSTokenLike(CoinJoinLike(coinJoin).systemCoin());

        _lockETH(manager, ethJoin, safe, subtract(msg.value, liquidityWad));

        _generateDebt(manager, taxCollector, coinJoin, safe, deltaWad, address(this));

        _provideLiquidityUniswap(coinJoin, uniswapRouter, deltaWad, liquidityWad, address(this), minTokenAmounts);

        _stakeInMine(incentives);

        // sending back any leftover tokens/eth, necessary to manage change from providing liquidity
        msg.sender.call{value: address(this).balance}("");
        systemCoin.transfer(msg.sender, systemCoin.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function lockETHGenerateDebtProvideLiquidityUniswap(
        address manager,
        address taxCollector,
        address ethJoin,
        address coinJoin,
        address uniswapRouter,
        uint safe,
        uint deltaWad,
        uint liquidityWad,
        uint[2] calldata minTokenAmounts
    ) external payable {
        DSTokenLike systemCoin = DSTokenLike(CoinJoinLike(coinJoin).systemCoin());

        _lockETH(manager, ethJoin, safe, subtract(msg.value, liquidityWad));

        _generateDebt(manager, taxCollector, coinJoin, safe, deltaWad, address(this));

        _provideLiquidityUniswap(coinJoin, uniswapRouter, deltaWad, liquidityWad, msg.sender, minTokenAmounts);

        // sending back any leftover tokens/eth, necessary to manage change from providing liquidity
        msg.sender.call{value: address(this).balance}("");
        systemCoin.transfer(msg.sender, systemCoin.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function migrateCampaign(address _oldIncentives, address _newIncentives) external {
        GebIncentivesLike incentives = GebIncentivesLike(_oldIncentives);
        GebIncentivesLike newIncentives = GebIncentivesLike(_newIncentives);
        require(incentives.stakingToken() == newIncentives.stakingToken(), "geb-incentives/mismatched-staking-tokens");
        DSTokenLike rewardToken = DSTokenLike(incentives.rewardsToken());
        DSTokenLike lpToken = DSTokenLike(incentives.stakingToken());
        incentives.exit();

        _stakeInMine(_newIncentives);

        rewardToken.transfer(msg.sender, rewardToken.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function openLockETHGenerateDebtProvideLiquidityStake(
        address manager,
        address taxCollector,
        address ethJoin,
        address coinJoin,
        address uniswapRouter,
        address incentives,
        bytes32 collateralType,
        uint256 deltaWad,
        uint256 liquidityWad,
        uint256[2] calldata minTokenAmounts
    ) external payable returns (uint safe) {
        safe = openSAFE(manager, collateralType, address(this));
        DSTokenLike systemCoin = DSTokenLike(CoinJoinLike(coinJoin).systemCoin());

        _lockETH(manager, ethJoin, safe, subtract(msg.value, liquidityWad));

        _generateDebt(manager, taxCollector, coinJoin, safe, deltaWad, address(this));

        _provideLiquidityUniswap(coinJoin, uniswapRouter, deltaWad, liquidityWad, address(this), minTokenAmounts);

        _stakeInMine(incentives);

        // sending back any leftover tokens/eth, necessary to manage change from providing liquidity
        msg.sender.call{value: address(this).balance}("");
        systemCoin.transfer(msg.sender, systemCoin.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function openLockETHGenerateDebtProvideLiquidityUniswap(
        address manager,
        address taxCollector,
        address ethJoin,
        address coinJoin,
        address uniswapRouter,
        bytes32 collateralType,
        uint deltaWad,
        uint liquidityWad,
        uint[2] calldata minTokenAmounts
    ) external payable returns (uint safe) {
        safe = openSAFE(manager, collateralType, address(this));
        DSTokenLike systemCoin = DSTokenLike(CoinJoinLike(coinJoin).systemCoin());

        _lockETH(manager, ethJoin, safe, subtract(msg.value, liquidityWad));

        _generateDebt(manager, taxCollector, coinJoin, safe, deltaWad, address(this));

        _provideLiquidityUniswap(coinJoin, uniswapRouter, deltaWad, liquidityWad, msg.sender, minTokenAmounts);

        // sending back any leftover tokens/eth, necessary to manage change from providing liquidity
        msg.sender.call{value: address(this).balance}("");
        systemCoin.transfer(msg.sender, systemCoin.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function provideLiquidityStake(
        address coinJoin,
        address uniswapRouter,
        address incentives,
        uint wad,
        uint[2] memory minTokenAmounts
    ) public payable {
        DSTokenLike systemCoin = DSTokenLike(CoinJoinLike(coinJoin).systemCoin());
        systemCoin.transferFrom(msg.sender, address(this), wad);
        _provideLiquidityUniswap(coinJoin, uniswapRouter, wad, msg.value, address(this), minTokenAmounts);

        _stakeInMine(incentives);

        // sending back any leftover tokens/eth, necessary to manage change from providing liquidity
        msg.sender.call{value: address(this).balance}("");
        systemCoin.transfer(msg.sender, systemCoin.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function provideLiquidityUniswap(address coinJoin, address uniswapRouter, uint wad, uint[2] calldata minTokenAmounts) external payable {
        DSTokenLike systemCoin = DSTokenLike(CoinJoinLike(coinJoin).systemCoin());
        systemCoin.transferFrom(msg.sender, address(this), wad);
        _provideLiquidityUniswap(coinJoin, uniswapRouter, wad, msg.value, msg.sender, minTokenAmounts);

        // sending back any leftover tokens/eth, necessary to manage change from providing liquidity
        msg.sender.call{value: address(this).balance}("");
        systemCoin.transfer(msg.sender, systemCoin.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function removeLiquidityUniswap(address uniswapRouter, address systemCoin, uint value, uint[2] calldata minTokenAmounts) external returns (uint amountA, uint amountB) {
        DSTokenLike(getWethPair(uniswapRouter, systemCoin)).transferFrom(msg.sender, address(this), value);
        return _removeLiquidityUniswap(uniswapRouter, systemCoin, value, msg.sender, minTokenAmounts);
    }

GebProxyIncentivesActions.sol

    function stakeInMine(address incentives, uint wad) external {
        DSTokenLike(GebIncentivesLike(incentives).stakingToken()).transferFrom(msg.sender, address(this), wad);
        _stakeInMine(incentives);
    }

GebProxyIncentivesActions.sol

    function withdrawAndHarvest(address incentives, uint value) external {
        GebIncentivesLike incentivesContract = GebIncentivesLike(incentives);
        DSTokenLike rewardToken = DSTokenLike(incentivesContract.rewardsToken());
        DSTokenLike lpToken = DSTokenLike(incentivesContract.stakingToken());
        incentivesContract.withdraw(value);
        getRewards(incentives);
        lpToken.transfer(msg.sender, lpToken.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function withdrawFromMine(address incentives, uint value) external {
        GebIncentivesLike incentivesContract = GebIncentivesLike(incentives);
        DSTokenLike lpToken = DSTokenLike(incentivesContract.stakingToken());
        incentivesContract.withdraw(value);
        lpToken.transfer(msg.sender, lpToken.balanceOf(address(this)));
    }

GebProxyIncentivesActions.sol

    function withdrawHarvestRemoveLiquidity(address incentives, address uniswapRouter, address systemCoin, uint value, uint[2] memory minTokenAmounts) public returns (uint amountA, uint amountB) {
        GebIncentivesLike incentivesContract = GebIncentivesLike(incentives);
        DSTokenLike rewardToken = DSTokenLike(incentivesContract.rewardsToken());
        DSTokenLike lpToken = DSTokenLike(incentivesContract.stakingToken());
        incentivesContract.withdraw(value);
        getRewards(incentives);
        rewardToken.transfer(msg.sender, rewardToken.balanceOf(address(this)));
        return _removeLiquidityUniswap(uniswapRouter, systemCoin, lpToken.balanceOf(address(this)), msg.sender, minTokenAmounts);
    }

GebProxyIncentivesActions.sol

    function withdrawRemoveLiquidityRepayDebt(address manager, address coinJoin, uint safe, address incentives, uint value, address uniswapRouter, uint[2] calldata minTokenAmounts) external {
        GebIncentivesLike incentivesContract = GebIncentivesLike(incentives);
        DSTokenLike rewardToken = DSTokenLike(incentivesContract.rewardsToken());
        DSTokenLike systemCoin = DSTokenLike(CoinJoinLike(coinJoin).systemCoin());
        incentivesContract.withdraw(value);

        _removeLiquidityUniswap(uniswapRouter, address(systemCoin), value, address(this), minTokenAmounts);
        _repayDebt(manager, coinJoin, safe, systemCoin.balanceOf(address(this)), false);
        rewardToken.transfer(msg.sender, rewardToken.balanceOf(address(this)));
        msg.sender.call{value: address(this).balance}("");
    }

GebProxyLeverageActions.sol

    function uniswapV2Call(address _sender, uint _amount0, uint _amount1, bytes calldata _data) external {
        require(_sender == proxy, "invalid sender");
        require(msg.sender == uniswapPair, "invalid uniswap pair");

        // transfer coins
        (address _tokenBorrow,,,,,,,address _proxy) = abi.decode(_data, (address, uint, address, bool, bool, bytes, address, address));
        DSTokenLike(_tokenBorrow).transfer(proxy, (_amount0 > 0) ? _amount0 : _amount1);

        // call proxy
        (bool success,) = proxy.call(abi.encodeWithSignature("execute(address,bytes)", _proxy, msg.data));
        require(success, "");
    }

GebProxyLeverageActions.sol

    function uniswapV2Call(address _sender, uint /* _amount0 */, uint /* _amount1 */, bytes calldata _data) external {
        require(_sender == address(this), "only this contract may initiate");
        DSAuth(address(this)).setAuthority(DSAuthority(address(0)));

        // decode data
        (
            address _tokenBorrow,
            uint _amount,
            address _tokenPay,
            bool _isBorrowingEth,
            bool _isPayingEth,
            bytes memory _userData,
            address weth,
            // address proxy
        ) = abi.decode(_data, (address, uint, address, bool, bool, bytes, address, address));

        // unwrap WETH if necessary
        if (_isBorrowingEth) {
            WethLike(weth).withdraw(_amount);
        }

        // compute the amount of _tokenPay that needs to be repaid
        // address pairAddress = permissionedPairAddress; // gas efficiency
        uint pairBalanceTokenBorrow = DSTokenLike(_tokenBorrow).balanceOf(FlashSwapProxy(msg.sender).uniswapPair());
        uint pairBalanceTokenPay = DSTokenLike(_tokenPay).balanceOf(FlashSwapProxy(msg.sender).uniswapPair());
        uint amountToRepay = ((1000 * pairBalanceTokenPay * _amount) / (997 * pairBalanceTokenBorrow)) + 1;

        // do whatever the user wants
        if (_isBorrowingEth)
            flashLeverageCallback(_amount, amountToRepay, _userData);
        else
            flashDeleverageCallback(_amount, amountToRepay, _userData);

        // payback loan
        // wrap ETH if necessary
        if (_isPayingEth) {
            WethLike(weth).deposit{value: amountToRepay}();
        }
        DSTokenLike(_tokenPay).transfer(FlashSwapProxy(msg.sender).uniswapPair(), amountToRepay);
    }

GebProxySaviourActions.sol

    function transferTokenFromAndApprove(address token, address target, uint256 amount) internal {
        DSTokenLike(token).transferFrom(msg.sender, address(this), amount);
        DSTokenLike(token).approve(target, 0);
        DSTokenLike(token).approve(target, amount);
    }

GebProxySaviourActions.sol

    function transferTokensToCaller(address[] memory tokens) public {
        for (uint i = 0; i < tokens.length; i++) {
            uint256 selfBalance = DSTokenLike(tokens[i]).balanceOf(address(this));
            if (selfBalance > 0) {
              DSTokenLike(tokens[i]).transfer(msg.sender, selfBalance);
            }
        }
    }

RISK ACCEPTED: The H2O team accepts the risk of this issue, claiming that all tokens in the system, i.e., the OCEAN collateral token, the stable coin and the governance token will be used in production revert on failure. Moreover, the team will add WARNING documentation to warn downstream users to use the H2O system only with tokens that revert to the failure.

Implementing a valid Access Control policy in smart contracts is essential to maintain security and decentralize permissions on a token. Moreover, access Control gives the features to mint/burn tokens and pause contracts. For instance, Ownership is the most common form of Access Control. In other words, the owner of a contract (the account that deployed it by default) can do some administrative tasks on it. Nevertheless, different authorization levels are required to keep the principle of least privilege, also known as least authority. Briefly, any process, user, or program can only access the necessary resources or information. Otherwise, the ownership role is beneficial in simple systems, but more complex projects require more roles using Role-based access control.

In most scope contracts, The authorizedAccounts of the contract are the accounts that control all privileged functions. Everything is managed and controlled by the Authorized accounts, with no other access control. If this account is compromised, then all functionalities would be controlled by an attacker, such as removing all other authorized accounts, changing price source to a malicious address, restarting feed and setting it to 0, starting and stopping the DSM OSM, etc.

Code Location

In-Scope Contracts

    constructor (address priceSource_) public {
        authorizedAccounts[msg.sender] = 1;

In-Scope Contracts

    constructor (address priceSource_, uint256 deviation) public {
        require(deviation > 0 && deviation < WAD, "DSM/invalid-deviation");

        authorizedAccounts[msg.sender] = 1;

PENDING: The team will fix the issue in a future release adding the below points.

• In case externally owned accounts (EOAs) are the authorized accounts, the team will implement RBAC according to the software engineering principle of separation of concerns by authorizing only a single proxy contract and implementing RBAC in the proxy contract configuration. For example, the DSRoles proxy contract implements permissions at an address method signature level.
• In other cases, if a smart contract is the authorized account; the team will only authorize smart contracts for which the code is known and trusted in advance.

It was identified that some in-scope contracts of H2O branch are missing nonReentrant guard. In these function, write of persistent state and external calls following an external call is identified, making it vulnerable to a Reentrancy attack.

• DebtCeilingProposal.sol contract function executeProposal missing nonReentrant guard.
• GlobalAuctionParamsProposal.sol contract function executeProposal missing nonReentrant guard.
• GlobalSettlementProposal.sol contract function executeProposal missing nonReentrant guard.

To protect against cross-function reentrancy attacks, it may be necessary to use a mutex. By using this lock, an attacker can no longer exploit the function with a recursive call. OpenZeppelin has its own mutex implementation called ReentrancyGuard which provides a modifier to any function called “nonReentrant” that guards the function with a mutex against the Reentrancy attacks.

Code Location

• state change executed = true following an external call pause.executeTransaction

DebtCeilingProposal.sol

    function executeProposal() public { // exec
        require(!executed, "proposal-already-executed");

        pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);

        executed = true;
    }

GlobalAuctionParamsProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        bytes memory signature =
            abi.encodeWithSignature(
                "modifyParameters(address,bytes32,uint256)",
                accountingEngine,
                bytes32("initialDebtAuctionMintedTokens"),
                initialDebtMintedTokens
        );
        pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);

        signature = abi.encodeWithSignature(
                "modifyParameters(address,bytes32,uint256)",
                accountingEngine,
                bytes32("debtAuctionBidSize"),
                debtAuctionBidSize
        );
        pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);

        executed = true;
    }

GlobalSettlementProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        bytes memory signature =
                abi.encodeWithSignature("shutdownSystem(address)", globalSettlement);

        pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);

        executed = true;
    }

SOLVED: The H2O team solved the above issue in the commit 384b866ebf808831d3f5980f4f4f62dc17d9365d. As a result, the team added the nonReentrant modifier.

In the scope contract FSMWrapper.sol, OSM.sol and DSM.sol, it is observed that the authorize accounts can set max and base rewards to an invalid value such as 0. It is possible, as no minimum and maximum value validation on the state variable baseUpdateCallerReward and maxUpdateCallerReward is performed.

Code Location

• Authorize accounts calling modifyParameters with byte32 value of baseUpdateCallerReward as a parameter and val as 0 sets baseUpdateCallerReward to 0. After this, call modifyParameters with byte32 value of maxUpdateCallerReward as a parameter and val as 0 sets maxUpdateCallerReward to 0.

FSMWrapper.sol, OSM.sol and DSM.sol

    function modifyParameters(bytes32 parameter, uint256 val) external isAuthorized {
        if (parameter == "baseUpdateCallerReward") {
          require(val <= maxUpdateCallerReward, "FSMWrapper/invalid-base-caller-reward");
          baseUpdateCallerReward = val;
        }
        else if (parameter == "maxUpdateCallerReward") {
          require(val >= baseUpdateCallerReward, "FSMWrapper/invalid-max-caller-reward");
          maxUpdateCallerReward = val;
        }

RISK ACCEPTED: The H2O team accepts the risk of this issue, claiming that for the above issue, in case the delay is set incorrectly, you can use the same function call to set it to a correct value. Additionally, the team can add an extra layer by including a multisig system.

In the scope contract FSMWrapper.sol, OSM.sol and DSM.sol, it is observed that the authorize accounts can set max reward increase delay to a vast number, as val is a type of uint256 a max value 2^256-1 could be supplied as an argument which ends up setting the state variable maxRewardIncreaseDelay to max INT value, this sets max reward not to increase for years.

Code Location

• Authorize accounts calling modifyParameters with byte32 value of maxRewardIncreaseDelay as a parameter and val as 2^256-1 sets maxRewardIncreaseDelay to max INT value.

FSMWrapper.sol, OSM.sol and DSM.sol

    function modifyParameters(bytes32 parameter, uint256 val) external isAuthorized {
        if (parameter == "baseUpdateCallerReward") {
          require(val <= maxUpdateCallerReward, "FSMWrapper/invalid-base-caller-reward");
          baseUpdateCallerReward = val;
        }
        else if (parameter == "maxUpdateCallerReward") {
          require(val >= baseUpdateCallerReward, "FSMWrapper/invalid-max-caller-reward");
          maxUpdateCallerReward = val;
        }
        else if (parameter == "perSecondCallerRewardIncrease") {
          require(val >= RAY, "FSMWrapper/invalid-caller-reward-increase");
          perSecondCallerRewardIncrease = val;
        }
        else if (parameter == "maxRewardIncreaseDelay") {
          require(val > 0, "FSMWrapper/invalid-max-increase-delay");
          maxRewardIncreaseDelay = val;
        }
        else if (parameter == "reimburseDelay") {
          reimburseDelay = val;
        }
        else revert("FSMWrapper/modify-unrecognized-param");
        emit ModifyParameters(
          parameter,
          val
        );
    }

RISK ACCEPTED: The H2O team accepts the risk of this issue, claiming that for the above issue, in case the delay is set incorrectly, you can use the same function call to set it to a correct value. Additionally, the team can add an extra layer by including a multisig system.

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero, and then the actual allowance must be approved.

IERC20(token).approve(address(operator), 0);
IERC20(token).approve(address(operator), amount);

Code Location

GebProxyIncentivesActions.sol

    function _removeLiquidityUniswap(address uniswapRouter, address systemCoin, uint value, address to, uint[2] memory minTokenAmounts) internal returns (uint amountA, uint amountB) {
        DSTokenLike(getWethPair(uniswapRouter, systemCoin)).approve(uniswapRouter, value);
        return IUniswapV2Router02(uniswapRouter).removeLiquidityETH(
            systemCoin,
            value,
            minTokenAmounts[0],
            minTokenAmounts[1],
            to,
            block.timestamp
        );
    }

GebProxyIncentivesActions.sol

    function _stakeInMine(address incentives) internal {
        DSTokenLike lpToken = DSTokenLike(GebIncentivesLike(incentives).stakingToken());
        lpToken.approve(incentives, uint(0 - 1));
        GebIncentivesLike(incentives).stake(lpToken.balanceOf(address(this)));
    }

RISK ACCEPTED: The H2O team accepts the risk of this issue, claiming that the issue will not be triggered as Uniswap LP tokens will be used in production. Moreover, the team will add documentation to warn downstream users using the H2O system only with tokens that do not exhibit this behavior in their approve functions.

The following contract functions perform an ERC20.approve() call, but does not check the success return value. Some tokens do not revert if the approval failed and return false instead of true.

Code Location

GebProxyActions.sol

    function increaseBidSize(address auctionHouse, uint auctionId, uint bidSize) public {
        SurplusAuctionHouseLike surplusAuctionHouse = SurplusAuctionHouseLike(auctionHouse);
        DSTokenLike protocolToken = DSTokenLike(surplusAuctionHouse.protocolToken());

        require(protocolToken.transferFrom(msg.sender, address(this), bidSize), "geb-proxy-auction-actions/transfer-from-failed");
        protocolToken.approve(address(surplusAuctionHouse), bidSize);
        // Restarts auction if inactive
        (, uint amountToSell,, uint48 bidExpiry, uint48 auctionDeadline) = surplusAuctionHouse.bids(auctionId);
        if (auctionDeadline < now && bidExpiry == 0 && auctionDeadline > 0) {
            surplusAuctionHouse.restartAuction(auctionId);
        }
        // Bid
        surplusAuctionHouse.increaseBidSize(auctionId, amountToSell, bidSize);
    }

GebProxyActions.sol

    function startAndIncreaseBidSize(address accountingEngineAddress, uint bidSize) public {
        AccountingEngineLike accountingEngine = AccountingEngineLike(accountingEngineAddress);
        SurplusAuctionHouseLike surplusAuctionHouse = SurplusAuctionHouseLike(accountingEngine.surplusAuctionHouse());
        DSTokenLike protocolToken = DSTokenLike(surplusAuctionHouse.protocolToken());

        // Starts auction
        uint auctionId = accountingEngine.auctionSurplus();
        require(protocolToken.transferFrom(msg.sender, address(this), bidSize), "geb-proxy-auction-actions/transfer-from-failed");
        protocolToken.approve(address(surplusAuctionHouse), bidSize);
        (, uint amountToSell,,,) = surplusAuctionHouse.bids(auctionId);
        // Bids
        surplusAuctionHouse.increaseBidSize(auctionId, amountToSell, bidSize);
    }

GebProxyIncentivesActions.sol

    function _provideLiquidityUniswap(address coinJoin, address uniswapRouter, uint tokenWad, uint ethWad, address to, uint[2] memory minTokenAmounts) internal {
        CoinJoinLike(coinJoin).systemCoin().approve(uniswapRouter, tokenWad);
        IUniswapV2Router02(uniswapRouter).addLiquidityETH{value: ethWad}(
            address(CoinJoinLike(coinJoin).systemCoin()),
            tokenWad,
            minTokenAmounts[0],
            minTokenAmounts[1],
            to,
            block.timestamp
        );
    }

GebProxyIncentivesActions.sol

    function _removeLiquidityUniswap(address uniswapRouter, address systemCoin, uint value, address to, uint[2] memory minTokenAmounts) internal returns (uint amountA, uint amountB) {
        DSTokenLike(getWethPair(uniswapRouter, systemCoin)).approve(uniswapRouter, value);
        return IUniswapV2Router02(uniswapRouter).removeLiquidityETH(
            systemCoin,
            value,
            minTokenAmounts[0],
            minTokenAmounts[1],
            to,
            block.timestamp
        );
    }

GebProxyIncentivesActions.sol

    function _stakeInMine(address incentives) internal {
        DSTokenLike lpToken = DSTokenLike(GebIncentivesLike(incentives).stakingToken());
        lpToken.approve(incentives, uint(0 - 1));
        GebIncentivesLike(incentives).stake(lpToken.balanceOf(address(this)));
    }

CollateralLike Abstract Interface

abstract contract CollateralLike {
    function approve(address, uint) virtual public;
    function transfer(address, uint) virtual public;
    function transferFrom(address, address, uint) virtual public;
}

DSTokenLike Abstract Interface

abstract contract DSTokenLike {
    function balanceOf(address) virtual public view returns (uint);
    function approve(address, uint) virtual public;
    function transfer(address, uint) virtual public returns (bool);
    function transferFrom(address, address, uint) virtual public returns (bool);
}

WethLike Abstract Interface

abstract contract WethLike {
    function balanceOf(address) virtual public view returns (uint);
    function approve(address, uint) virtual public;
    function transfer(address, uint) virtual public;
    function transferFrom(address, address, uint) virtual public;
    function deposit() virtual public payable;
    function withdraw(uint) virtual public;
}

RISK ACCEPTED: The H2O team accepts the risk of this issue, claiming that all tokens to be used in production will be reverted in the event of a failure. Moreover, the team will add documentation to warn downstream users to use the H2O system only with tokens that will revert the ERC20.approve().

It was noted that safeEngine_ address is not validated in SurplusAuctionHouse.sol contract. Lack of address validation has been found when assigning supplied address values to state variables directly. safeEngine_ address should be !=0 or add logic to check if the provided address is a valid Safe Engine Address.

Code Location

SurplusAuctionHouse.sol

    constructor(address safeEngine_, address protocolToken_) public {
        authorizedAccounts[msg.sender] = 1;
        safeEngine = SAFEEngineLike(safeEngine_);
        protocolToken = TokenLike(protocolToken_);
        contractEnabled = 1;
        emit AddAuthorization(msg.sender);
    }

SAFEEngine.sol

    constructor() public {
        authorizedAccounts[msg.sender] = 1;
        safeDebtCeiling = uint256(-1);
        contractEnabled = 1;
        emit AddAuthorization(msg.sender);
        emit ModifyParameters("safeDebtCeiling", uint256(-1));
    }

SOLVED: The H2O team solved the above issue in the commit 9c3225a836a570df867a8116eeac6f678b322a13. As a result, the team added the zero address check to safeEngine_ throughout the in-scope H2O system.

It was noted that factory_ address is not validated in GebProxyRegistry.sol contract. Lack of address validation has been found when assigning supplied address values to state variables directly. factory_ address should be !=0 or add logic to check if the provided address is a valid Factory Address.

Code Location

GebProxyRegistry.sol

    constructor(address factory_) public {
        factory = DSProxyFactory(factory_);
    }

proxy.sol

contract DSProxyFactory {
    event Created(address indexed sender, address indexed owner, address proxy, address cache);
    mapping(address=>bool) public isProxy;
    DSProxyCache public cache;

    constructor() public {
        cache = new DSProxyCache();
    }

SOLVED: The H2O team solved the above issue in the commit 48863e0b91e152c6900fb6d61d31566fa9819bb1. As a result, the team added the zero address check to factory_.

Updated Code

    constructor(address factory_) public {
        require(factory_ != address(0), "GebProxyRegistry/proxy-factory-address-invalid");
        factory = DSProxyFactory(factory_);
    }

External calls inside a loop increase Gas usage or might lead to a denial-of-service attack. In GebProxySaviourActions.sol contract functions discovered, there is a for loop on variable i that iterates up to the tokens.length array length. Where, in AccessManagementProposal.sol, CollateralStabilityFeeProposal.sol, LiquidationCRatioProposal.sol, LiquidationPenaltyProposal.sol, MultiDebtCeilingProposal.sol, SafetyCRatioProposal.sol, SecondaryTaxReceiversProposal.sol contract functions discovered there is a for loop on variable i that iterates up to the gebModules.length, and collateralTypes.length array length and these loop has external calls inside a loop. If this integer is evaluated at huge numbers, this can cause a DoS.

Code Location

GebProxySaviourActions.sol

    function transferTokensToCaller(address[] memory tokens) public {
        for (uint i = 0; i < tokens.length; i++) {
            uint256 selfBalance = DSTokenLike(tokens[i]).balanceOf(address(this));
            if (selfBalance > 0) {
              DSTokenLike(tokens[i]).transfer(msg.sender, selfBalance);
            }
        }
    }

AccessManagementProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < gebModules.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    (grantAccess[i] != 0) ? "addAuthorization(address,address)" : "removeAuthorization(address,address)",
                    gebModules[i],
                    addresses[i]
            );

            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

AccessManagementProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < gebModules.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    (grantAccess[i] != 0) ? "addAuthorization(address,address)" : "removeAuthorization(address,address)",
                    gebModules[i],
                    addresses[i]
            );

            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

CollateralStabilityFeeProposal.sol

    function deploy(address taxCollector, bytes32[] calldata collateralTypes, uint256[] calldata stabilityFees) external {

        for (uint i = 0; i < collateralTypes.length; i++) 
            ConfigLike(taxCollector).modifyParameters(collateralTypes[i], "stabilityFee", stabilityFees[i]); 
    }

LiquidationCRatioProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("liquidationCRatio"),
                    liquidationCRatios[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

LiquidationCRatioProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("liquidationCRatio"),
                    liquidationCRatios[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

LiquidationPenaltyProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    liquidationEngine,
                    collateralTypes[i],
                    bytes32("liquidationPenalty"),
                    liquidationPenalties[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

LiquidationPenaltyProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    liquidationEngine,
                    collateralTypes[i],
                    bytes32("liquidationPenalty"),
                    liquidationPenalties[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

MultiDebtCeilingProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    safeEngine,
                    collateralTypes[i],
                    bytes32("debtCeiling"),
                    debtCeilings[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

MultiDebtCeilingProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    safeEngine,
                    collateralTypes[i],
                    bytes32("debtCeiling"),
                    debtCeilings[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

SafetyCRatioProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("safetyCRatio"),
                    safetyCRatios[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

SafetyCRatioProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("safetyCRatio"),
                    safetyCRatios[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

SecondaryTaxReceiversProposal.sol

    function deploy(
        address taxCollector, 
        bytes32[] calldata collateralTypes, 
        uint256[] calldata positions, 
        uint256[] calldata percentages, 
        address[] calldata secondaryReceivers) 
        external 
    {

        for (uint i = 0; i < collateralTypes.length; i++) 
            ConfigLike(taxCollector).modifyParameters(collateralTypes[i], positions[i], percentages[i], secondaryReceivers[i]); 
    }

RISK ACCEPTED: The H2O team accepts the risk of this issue, claiming that for V1 of the system, the identified array lengths will not exceed more than a handful of values. However, in the case of collateralTypes, the length of the array is fixed at 1 for the lifetime of the V1 system. Furthermore, the team claims that the contracts in which these issues occur are optional and may not be seen in the deployment for the lifetime of V1. However, the team notified that in the V2 system, the team would address these issues.

The return value of an external call is not stored in a local or state variable. In contract GebProxyIncentivesActions.sol, there is an instance where external method is being called, and return value is being ignored.

Code Location

GebProxyIncentivesActions.sol

    function _provideLiquidityUniswap(address coinJoin, address uniswapRouter, uint tokenWad, uint ethWad, address to, uint[2] memory minTokenAmounts) internal {
        CoinJoinLike(coinJoin).systemCoin().approve(uniswapRouter, tokenWad);
        IUniswapV2Router02(uniswapRouter).addLiquidityETH{value: ethWad}(
            address(CoinJoinLike(coinJoin).systemCoin()),
            tokenWad,
            minTokenAmounts[0],
            minTokenAmounts[1],
            to,
            block.timestamp
        );
    }

RISK ACCEPTED: The H2O team accepts the risk of this issue, claiming that the implementation of addLiquidityETH throws on failure unless the codebase is used with an alternative implementation of Uniswap that doesn’t exhibit this behavior. Furthermore, the team claims that only addLiquidityETH implementations that revert on failure are used with this codebase. Moreover, the team will add documentation to WARN downstream users of this assumption.

There are multiple instances found where Address validation is missing. Lack of zero address validation has been found when assigning user supplied address values to state variables directly.

• In contract AccessManagementProposal.sol:

  • constructor lacks a zero address check on _target.

• In contract CollateralStabilityFeeProposal.sol:

  • constructor lacks a zero address check on _pause.

• In contract DebtCeilingProposal.sol:

  • constructor lacks a zero address check on _target, _cdpEngine.

• In contract GlobalAuctionParamsProposal.sol:

  • constructor lacks a zero address check on _target, _accountingEngine.

• In contract GlobalSettlementProposal.sol:

  • constructor lacks a zero address check on _target, _globalSettlement.

• In contract GlobalStabilityFeeProposal.sol:

  • constructor lacks a zero address check on _pause.

• In contract LiquidationCRatioProposal.sol:

  • constructor lacks a zero address check on _target, _oracleRelayer.

• In contract LiquidationPenaltyProposal.sol:

  • constructor lacks a zero address check on _target, _liquidationEngine.

• In contract MultiDebtCeilingProposal.sol:

  • constructor lacks a zero address check on _target, _safeEngine.

• In contract NewCollateralProposal.sol:

  • constructor lacks a zero address check on pause_.

• In contract Proposal.sol:

  • constructor lacks a zero address check on target_.

• In contract SafetyCRatioProposal.sol:

  • constructor lacks a zero address check on _target, _oracleRelayer.

• In contract SecondaryTaxReceiversProposal.sol:

  • constructor lacks a zero address check on _pause.

• In contract GebProxyLeverageActions.sol:

  • constructor lacks a zero address check on _pair.
  • uniswapV2Call lacks a zero address check on _proxy.

• constructor lacks a zero address check on _target.

• constructor lacks a zero address check on _pause.

• constructor lacks a zero address check on _target, _cdpEngine.

• constructor lacks a zero address check on _target, _accountingEngine.

• constructor lacks a zero address check on _target, _globalSettlement.

• constructor lacks a zero address check on _pause.

• constructor lacks a zero address check on _target, _oracleRelayer.

• constructor lacks a zero address check on _target, _liquidationEngine.

• constructor lacks a zero address check on _target, _safeEngine.

• constructor lacks a zero address check on pause_.

• constructor lacks a zero address check on target_.

• constructor lacks a zero address check on _target, _oracleRelayer.

• constructor lacks a zero address check on _pause.

• constructor lacks a zero address check on _pair.

• uniswapV2Call lacks a zero address check on _proxy.

Code Location

Zero Address Validation missing before address assignment to this state variable.

AccessManagementProposal.sol

 target = _target (#47)

CollateralStabilityFeeProposal.sol

 pause = _pause (#43)

DebtCeilingProposal.sol

 target = _target (#44)
 cdpEngine = _cdpEngine (#45)

GlobalAuctionParamsProposal.sol

 target = _target (#45)
 accountingEngine = _accountingEngine (#46)

GlobalSettlementProposal.sol

 target = _target (#39)
 globalSettlement = _globalSettlement (#40)

GlobalStabilityFeeProposal.sol

 pause = _pause (#37)

LiquidationCRatioProposal.sol

 target = _target (#46)
 oracleRelayer = _oracleRelayer (#47)

LiquidationPenaltyProposal.sol

 target = _target (#46)
 liquidationEngine = _liquidationEngine (#47)

MultiDebtCeilingProposal.sol

 target = _target (#47)
 safeEngine = _safeEngine (#48)

NewCollateralProposal.sol

 pause = pause_ (#76)

Proposal.sol

 target = target_ (#30)

SafetyCRatioProposal.sol

 target = _target (#46)
 oracleRelayer = _oracleRelayer (#47)

SecondaryTaxReceiversProposal.sol

 pause = _pause (#60)

RISK ACCEPTED: The H2O team accepts the risk of this issue, claiming that these state variables are declared and assigned in scripts that mitigate human error compared to calling these functions manually; as a result, they do not appear to be a problem in practice.

In the entire smart contracts, Instead of block.timestamp, now is used, which is deprecated. block.timestamp is way more explicit in showing the intent, while now relates to the timestamp of the block controlled by the miner. Reference

Code Location

LiquidationPenaltyProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    liquidationEngine,
                    collateralTypes[i],
                    bytes32("liquidationPenalty"),
                    liquidationPenalties[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

MultiDebtCeilingProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    safeEngine,
                    collateralTypes[i],
                    bytes32("debtCeiling"),
                    debtCeilings[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

MultiDebtCeilingProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    safeEngine,
                    collateralTypes[i],
                    bytes32("debtCeiling"),
                    debtCeilings[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

SOLVED: The H2O team solved the above issue in the commit 54690e489294a144e56659b117ced4ac11de4400. As a result, the team replaces now with block.timestamp throughout the in-scope H2O system.

In the contract, SurplusAuctionHouse.sol observed that the function restartAuction lacks access control; thus, anyone can make an external call to this function. Furthermore, calling the restartAuction function just before the auction expires (no bid is issued) allows the auction to be extended by two days. An external call to restartAuction only requires a valid auction id, the auction id can easily be obtainable via the startAuction function emit event. This can be done repeatedly to extend the auction forever for any auction.

Code Location

• Default Auction length is of two days

```{language=solidity firstnumber=297 caption=SurplusAuctionHouse.sol hlines=298} // Total length of the auction uint48 public totalAuctionLength = 2 days;

- Active auction's `id` can be obtained via emit event of `startAuction` function

```{language=solidity firstnumber=386 caption=SurplusAuctionHouse.sol hlines=386}
        emit StartAuction(id, auctionsStarted, amountToSell, initialBid, bids[id].auctionDeadline);

• Condition bids[id].auctionDeadline < now will always be true if restartAuction function is called just before an auction expires, as a result, bids[id].auctionDeadline = addUint48(uint48(now), totalAuctionLength); increase the auction length again by 2 days.

SurplusAuctionHouse.sol

    function restartAuction(uint256 id) external {
        require(bids[id].auctionDeadline < now, "RecyclingSurplusAuctionHouse/not-finished");
        require(bids[id].bidExpiry == 0, "RecyclingSurplusAuctionHouse/bid-already-placed");
        bids[id].auctionDeadline = addUint48(uint48(now), totalAuctionLength);
        emit RestartAuction(id, bids[id].auctionDeadline);
    }

ACKNOWLEDGED: The team claims the above issue is intended behavior. Moreover, the team will add the intended behavior note in the README documentation. Furthermore, the team added that, similar to MakerDAO and RAI, the H2O protocol is optimized to maximize the final bid amount rather than minimize the auction time. Therefore, auctions can be extended if there is no bid as this does not affect the outcome of other auctions or the operation of other parts of the protocol.

In the contract, SurplusAuctionHouse.sol observed no logic to stop a seller from being a buyer. Following HAL-16, a seller can restart if no bid is placed before the auction closes, resulting in an extension of the auction time by two days. Furthermore, If a valid buyer comes and sets a new bid, then just before the bid expires, a seller can raise the bid again via an external call to increaseBidSize and extend the bid duration by the following three hours. This scenario can continue, as the contract does not prevent the seller from becoming the buyer and controlling the auction's bid price.

Code Location

• Default extension to bid expiry after a new bid is submitted

SurplusAuctionHouse.sol

    uint48   public   bidDuration = 3 hours;   

• No logic to stop seller from being a buyer now if a valid buyer comes and places a new bid, then just before bid expires, a seller can raise the bid again and extend the bid duration by next three hours. The scenario can go on until the auction expires.

SurplusAuctionHouse.sol

    function increaseBidSize(uint256 id, uint256 amountToBuy, uint256 bid) external {
        require(contractEnabled == 1, "RecyclingSurplusAuctionHouse/contract-not-enabled");
        require(bids[id].highBidder != address(0), "RecyclingSurplusAuctionHouse/high-bidder-not-set");
        require(bids[id].bidExpiry > now || bids[id].bidExpiry == 0, "RecyclingSurplusAuctionHouse/bid-already-expired");
        require(bids[id].auctionDeadline > now, "RecyclingSurplusAuctionHouse/auction-already-expired");

        require(amountToBuy == bids[id].amountToSell, "RecyclingSurplusAuctionHouse/amounts-not-matching");
        require(bid > bids[id].bidAmount, "RecyclingSurplusAuctionHouse/bid-not-higher");
        require(multiply(bid, ONE) >= multiply(bidIncrease, bids[id].bidAmount), "RecyclingSurplusAuctionHouse/insufficient-increase");

        if (msg.sender != bids[id].highBidder) {
            protocolToken.move(msg.sender, bids[id].highBidder, bids[id].bidAmount);
            bids[id].highBidder = msg.sender;
        }
        protocolToken.move(msg.sender, address(this), bid - bids[id].bidAmount);

        bids[id].bidAmount = bid;
        bids[id].bidExpiry = addUint48(uint48(now), bidDuration);

        emit IncreaseBidSize(id, msg.sender, amountToBuy, bid, bids[id].bidExpiry);
    }

ACKNOWLEDGED: The team claims the above issue is intended behavior. Moreover, the team will add the intended behavior note in the README documentation. Furthermore, the team added that, the protocol intends to maximize the amount of capital raised through the auction mechanism rather than minimize the auction duration or restrict participation; therefore, limiting any means of increasing the bid price is undesirable.

In public functions, array arguments are immediately copied to memory, while external functions can read directly from calldata. Reading calldata is cheaper than memory allocation. Public functions need to write the arguments to memory because public functions may be called internally. Internal calls are passed internally by pointers to memory. Thus, the function expects its arguments being located in memory when the compiler generates the code for an internal function. Furthermore, methods do not necessarily have to be public if they are only called within the contract-in such case they should be marked internal.

Code Location

Below are smart contracts and their corresponding functions affected: \textbf{\underline{AccessManagementProposal.sol}:} executeProposal() scheduleProposal()

\textbf{\underline{CollateralStabilityFeeProposal.sol}:} executeProposal()

\textbf{\underline{ConfigLike.sol}:} addAuthorization(address) initializeCollateralType(bytes32) modifyParameters(bytes32,bytes32,address) modifyParameters(bytes32,bytes32,uint256) modifyParameters(bytes32,uint256) modifyParameters(bytes32,uint256,uint256,address)

\textbf{\underline{DebtCeilingProposal.sol}:} executeProposal() scheduleProposal()

\textbf{\underline{DeployCollateralAuctionThottler.sol}:} execute(address,address,address)

\textbf{\underline{DeployDebtPopperRewards.sol}:} execute(address,address)

\textbf{\underline{DeployDSMandWrapper.sol}:} execute(address,address,address,bytes32,uint256)

\textbf{\underline{DeployGlobalSettlement.sol}:} execute(address)

\textbf{\underline{DeployIncreasingDiscountCollateralHouse.sol}:} execute(address,LiquidationEngineLike,bytes32,address)

\textbf{\underline{DeployOSMandWrapper.sol}:} execute(address,address,address)

\textbf{\underline{DeployPIRateSetter.sol}:} execute(address)

\textbf{\underline{DeploySingleSpotDebtCeilingSetter.sol}:} execute(address,address,address,bytes32,address)

\textbf{\underline{DeployUniswapMedianizer.sol}:} execute(address,address,address,address,address,uint256,uint256)

\textbf{\underline{DeployUniswapTWAP.sol}:} execute(address)

\textbf{\underline{GlobalAuctionParamsProposal.sol}:} executeProposal() scheduleProposal()

\textbf{\underline{GlobalSettlementProposal.sol}:} executeProposal() scheduleProposal()

\textbf{\underline{GlobalStabilityFeeProposal.sol}:} executeProposal()

\textbf{\underline{LiquidationCRatioProposal.sol}:} executeProposal() scheduleProposal()

\textbf{\underline{LiquidationPenaltyProposal.sol}:} executeProposal() scheduleProposal()

\textbf{\underline{MerkleDistributionManagement.sol}:} deployDistributor(address,bytes32,uint256,bool) dropDistributorAuth(address,uint256) getBackTokensFromDistributor(address,uint256,uint256) sendTokensToCustom(address,address,uint256) sendTokensToDistributor(address,uint256)

\textbf{\underline{MultiDebtCeilingProposal.sol}:} executeProposal() scheduleProposal()

\textbf{\underline{NewCollateralProposal.sol}:} executeProposal()

\textbf{\underline{OldRateSetterLike.sol}:} treasury()

\textbf{\underline{OldTwapLike.sol}:} treasury()

\textbf{\underline{OracleRelayerLike.sol}:} updateCollateralPrice(bytes32)

\textbf{\underline{PauseLike.sol}:} delay() executeTransaction(address,bytes32,bytes,uint256) scheduleTransaction(address,bytes32,bytes,uint256)

\textbf{\underline{Proposal.sol}:} executeProposal() newProposal(address,uint256,bytes) scheduleProposal()

\textbf{\underline{ProposalFactory.sol}:} newProposal(address,uint256,bytes)

\textbf{\underline{SafetyCRatioProposal.sol}:} executeProposal() scheduleProposal()

\textbf{\underline{SecondaryTaxReceiversProposal.sol}:} executeProposal()

\textbf{\underline{SwapGlobalSettlement.sol}:} execute(address,address)

ACKNOWLEDGED: The H2O team acknowledged this issue.

In the loop below, the variable i is incremented using i++. It is known that, in loops, using ++i costs less gas per iteration than i++.

Code Location

AccessManagementProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < gebModules.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    (grantAccess[i] != 0) ? "addAuthorization(address,address)" : "removeAuthorization(address,address)",
                    gebModules[i],
                    addresses[i]
            );

            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

AccessManagementProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < gebModules.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    (grantAccess[i] != 0) ? "addAuthorization(address,address)" : "removeAuthorization(address,address)",
                    gebModules[i],
                    addresses[i]
            );

            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

CollateralStabilityFeeProposal.sol

    function deploy(address taxCollector, bytes32[] calldata collateralTypes, uint256[] calldata stabilityFees) external {

        for (uint i = 0; i < collateralTypes.length; i++) 
            ConfigLike(taxCollector).modifyParameters(collateralTypes[i], "stabilityFee", stabilityFees[i]); 
    }

LiquidationCRatioProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("liquidationCRatio"),
                    liquidationCRatios[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

LiquidationCRatioProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("liquidationCRatio"),
                    liquidationCRatios[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

LiquidationPenaltyProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    liquidationEngine,
                    collateralTypes[i],
                    bytes32("liquidationPenalty"),
                    liquidationPenalties[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

LiquidationPenaltyProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    liquidationEngine,
                    collateralTypes[i],
                    bytes32("liquidationPenalty"),
                    liquidationPenalties[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

MultiDebtCeilingProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    safeEngine,
                    collateralTypes[i],
                    bytes32("debtCeiling"),
                    debtCeilings[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

MultiDebtCeilingProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    safeEngine,
                    collateralTypes[i],
                    bytes32("debtCeiling"),
                    debtCeilings[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

SafetyCRatioProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("safetyCRatio"),
                    safetyCRatios[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

SafetyCRatioProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("safetyCRatio"),
                    safetyCRatios[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

SecondaryTaxReceiversProposal.sol

    function deploy(
        address taxCollector, 
        bytes32[] calldata collateralTypes, 
        uint256[] calldata positions, 
        uint256[] calldata percentages, 
        address[] calldata secondaryReceivers) 
        external 
    {

        for (uint i = 0; i < collateralTypes.length; i++) 
            ConfigLike(taxCollector).modifyParameters(collateralTypes[i], positions[i], percentages[i], secondaryReceivers[i]); 
    }

For example, based in the following test contract:

Test.sol

//SPDX-License-Identifier: MIT
pragma solidity 0.8.9;

contract test {
    function postiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; i++) {
        }
    }
    function preiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; ++i) {
        }
    }
}

We can see the difference in the gas costs:

ACKNOWLEDGED: The H2O team acknowledged this issue.

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

Code Location

AccessManagementProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < gebModules.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    (grantAccess[i] != 0) ? "addAuthorization(address,address)" : "removeAuthorization(address,address)",
                    gebModules[i],
                    addresses[i]
            );

            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

AccessManagementProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < gebModules.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    (grantAccess[i] != 0) ? "addAuthorization(address,address)" : "removeAuthorization(address,address)",
                    gebModules[i],
                    addresses[i]
            );

            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

CollateralStabilityFeeProposal.sol

    function deploy(address taxCollector, bytes32[] calldata collateralTypes, uint256[] calldata stabilityFees) external {

        for (uint i = 0; i < collateralTypes.length; i++) 
            ConfigLike(taxCollector).modifyParameters(collateralTypes[i], "stabilityFee", stabilityFees[i]); 
    }

LiquidationCRatioProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("liquidationCRatio"),
                    liquidationCRatios[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

LiquidationCRatioProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("liquidationCRatio"),
                    liquidationCRatios[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

LiquidationPenaltyProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    liquidationEngine,
                    collateralTypes[i],
                    bytes32("liquidationPenalty"),
                    liquidationPenalties[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

LiquidationPenaltyProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    liquidationEngine,
                    collateralTypes[i],
                    bytes32("liquidationPenalty"),
                    liquidationPenalties[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

MultiDebtCeilingProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    safeEngine,
                    collateralTypes[i],
                    bytes32("debtCeiling"),
                    debtCeilings[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

MultiDebtCeilingProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    safeEngine,
                    collateralTypes[i],
                    bytes32("debtCeiling"),
                    debtCeilings[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

SafetyCRatioProposal.sol

    function executeProposal() public {
        require(!executed, "proposal-already-executed");

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("safetyCRatio"),
                    safetyCRatios[i]
            );
            pause.executeTransaction(target, codeHash, signature, earliestExecutionTime);
        }

        executed = true;
    }

SafetyCRatioProposal.sol

    function scheduleProposal() public {
        require(earliestExecutionTime == 0, "proposal-already-scheduled");
        earliestExecutionTime = now + PauseLike(pause).delay();

        for (uint256 i = 0; i < collateralTypes.length; i++) {
            bytes memory signature =
                abi.encodeWithSignature(
                    "modifyParameters(address,bytes32,bytes32,uint256)",
                    oracleRelayer,
                    collateralTypes[i],
                    bytes32("safetyCRatio"),
                    safetyCRatios[i]
            );
            pause.scheduleTransaction(target, codeHash, signature, earliestExecutionTime);
        }
    }

SecondaryTaxReceiversProposal.sol

    function deploy(
        address taxCollector, 
        bytes32[] calldata collateralTypes, 
        uint256[] calldata positions, 
        uint256[] calldata percentages, 
        address[] calldata secondaryReceivers) 
        external 
    {

        for (uint i = 0; i < collateralTypes.length; i++) 
            ConfigLike(taxCollector).modifyParameters(collateralTypes[i], positions[i], percentages[i], secondaryReceivers[i]); 
    }

ACKNOWLEDGED: The H2O team acknowledged this issue.

Gas optimizations and additional safety checks are available for free when using newer compiler versions and the optimizer.

• Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
• Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
• Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases, used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
• Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Code Location

All Contracts

ACKNOWLEDGED: The H2O team acknowledged this issue.

In the EVM, the exponentiation costs 10 gas, using the 1eX implementation can improve the gas usage.

Code Location

Location

PRawPerSecondCalculator.sol

    uint256 internal constant TWENTY_SEVEN_DECIMAL_NUMBER = 10 ** 27;
    uint256 internal constant EIGHTEEN_DECIMAL_NUMBER     = 10 ** 18;

SOLVED: The H2O team solved the above issue in the commit b49ec9ba27ad92e10f1a244078c343c2393773a0. As a result, the team replaced exponentiation with scientific notation.

Updated Code

    uint256 internal constant TWENTY_SEVEN_DECIMAL_NUMBER = 1e27;
    uint256 internal constant EIGHTEEN_DECIMAL_NUMBER     = 1e18;

In Solidity, the use of low-level calls could cause unexpected behavior if the call fails or an attacker provokes the call to fail. Then, it is a good practice not to use low-level calls to avoid a potentially exploitable behaviour. If the return value of a message call is not checked, the called contract may throw an exception.

Code Location

GebProxyIncentivesActions.sol

        // sending back any leftover tokens/eth, necessary to manage change from providing liquidity
        msg.sender.call{value: address(this).balance}("");
        systemCoin.transfer(msg.sender, systemCoin.balanceOf(address(this)));

SOLVED: The H2O team solved the above issue in the commit 593a0388dd719aa475d4c85a38c7fd6c05b56a8c. As a result, the team added the return value check that reverts if the return of funds fails.

Updated Code

        ( bool success , ) = msg.sender.call{value: address(this).balance}("");
        if (!success)
            revert("GebProxyIncentivesActions/failed-to-return-eth");
        systemCoin.transfer(msg.sender, systemCoin.balanceOf(address(this)));