wrong defaultCurrency can be set in the OptimisticOracleIntegrator causing the defaultBond to be zero

[hats-bug-reporter]

Github username: @Audinarey
Twitter username: audinarey
Submission hash (on-chain): 0xf2bc1358baf543cbfb82574e86088600a06d592dc42e108b446ec1c98a776254
Severity: medium

Description:
Description
From the UMA Optimistic Oracle Docs.

_defaultCurrency parameter in the constructor identifies the token used for bonds of data assertions. .This token should be approved as whitelisted UMA collateral. Please check Approved Collateral Types for production networks or call getWhitelist() on the Address Whitelist contract for any of the test networks. ...Alternatively, you can approve a new token address with addToWhitelist method in the Address Whitelist contract if working in a sandboxed UMA environment.

The OptimisticOracleIntegrator::_setDefaultCurrencyAndBond(...) function is used to set the defaultCurrency of the oracle integrator and the defaultBond is derived from the defaultCurrency.

However as shown below, the _setDefaultCurrencyAndBond function only checks that the defaultCurrency is not a zero address and a such a non-whitelisted currency can be set as default, ( when this happens, the getMinimumBond(...) function will return zero as well return zero because minimum bond is not set for the non whitelisted currency.

File: OptimisticOracleIntegrator.sol
138:     function _setDefaultCurrencyAndBond(address _newCurrency, uint _newBond)
139:         internal
140:     {
141:   >    if (address(_newCurrency) == address(0)) { // @audit missing check for whitelisted currency
142:             revert Module__OptimisticOracleIntegrator__InvalidDefaultCurrency(); // @audit SUGGESTION: add a check to ensure the currency is whitelisted
143:         }
144:         if (_newBond < oo.getMinimumBond(address(_newCurrency))) {
145:             revert Module__OptimisticOracleIntegrator__CurrencyBondTooLow();
146:         }
147: 
148:         defaultCurrency = IERC20(_newCurrency);
149:         defaultBond = _newBond;
150:     }
151. 

File: OptimisticOracleV3.sol
361:     function getMinimumBond(address currency) public view returns (uint256) {
362:  >     uint256 finalFee = cachedCurrencies[currency].finalFee;
363:         return (finalFee * 1e18) / burnedBondPercentage;
364:     }

Attack Scenario\

Attachments

2. Revised Code File (Optional)

Modify the OptimisticOracleIntegrator::_setDefaultCurrencyAndBond(...) function as shown below

File: OptimisticOracleIntegrator.sol
138:     function _setDefaultCurrencyAndBond(address _newCurrency, uint _newBond)
139:         internal
140:     {
141.   -     if (address(_newCurrency) == address(0)) 
141:   +    if (!oo.cachedCurrencies(address(_newCurrency)).isWhiteListed) { 
142:             revert Module__OptimisticOracleIntegrator__InvalidDefaultCurrency(); 
143:         }
144:         if (_newBond < oo.getMinimumBond(address(_newCurrency))) {
145:             revert Module__OptimisticOracleIntegrator__CurrencyBondTooLow();
146:         }
147: 
148:         defaultCurrency = IERC20(_newCurrency);
149:         defaultBond = _newBond;
150:     }
151. 

[NicolaMirchev]

Hey.

I can't entirely agree with the severity. The following does not have any impact. It is triggered by the admin function and even if it was by mistake, it can be changed at any time.
Also, I think this is a dup of #114 , which is marked Invalid