Open Dollar - klau5's results

Judge has assessed an item in Issue #165 as 2 risk. The relevant finding follows:

Allowed user have too much priviledge Links to affected code https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L105

Impact Allowed user can revoke approval of other allowed address.

Proof of Concept The allowed user can allow other user. If allowed address is compromised, it can cancel approval of other allowed user.

modifier safeAllowed(uint256 _safe) { address _owner = _safeData[_safe].owner; if (msg.sender != _owner && safeCan[_owner][_safe][msg.sender] == 0) revert SafeNotAllowed(); _; }

function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external safeAllowed(_safe) { address _owner = _safeData[_safe].owner; safeCan[_owner][_safe][_usr] = _ok; emit AllowSAFE(msg.sender, _safe, _usr, _ok); } Tools Used Manual Review

Recommended Mitigation Steps Restrict only the owner to allowSAFE.

• function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external safeAllowed(_safe) {

• function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external { address _owner = _safeData[_safe].owner;
• require(msg.sender == _owner, "not owner"); safeCan[_owner][_safe][_usr] = _ok; emit AllowSAFE(msg.sender, _safe, _usr, _ok); }

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L207-L212

Vulnerability details

Impact

If src has less lockedCollateral and generatedDebt than safeHandler, enterSystem cannot succeed.

The enterSystem function is behaving differently than intended.

Proof of Concept

The ODSafeManager.enterSystem function retrieves the amount of safeHandler's lockedCollateral and generatedDebt and sends it to safeHandler. The purpose of this function is to send _src's lockedCollateral and generatedDebt to safeHandler, so it is sending wrong amounts.

  function enterSystem(address _src, uint256 _safe) external handlerAllowed(_src) safeAllowed(_safe) {
    SAFEData memory _sData = _safeData[_safe];
@>  ISAFEEngine.SAFE memory _safeInfo = ISAFEEngine(safeEngine).safes(_sData.collateralType, _sData.safeHandler);
@>  int256 _deltaCollateral = _safeInfo.lockedCollateral.toInt();
@>  int256 _deltaDebt = _safeInfo.generatedDebt.toInt();
    ISAFEEngine(safeEngine).transferSAFECollateralAndDebt(
@>    _sData.collateralType, _src, _sData.safeHandler, _deltaCollateral, _deltaDebt
    );
    emit EnterSystem(msg.sender, _src, _safe);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Use lockedCollateral and generatedDebt of _src.

function enterSystem(address _src, uint256 _safe) external handlerAllowed(_src) safeAllowed(_safe) {
  SAFEData memory _sData = _safeData[_safe];
+ ISAFEEngine.SAFE memory _safeInfo = ISAFEEngine(safeEngine).safes(_sData.collateralType, _src);
- ISAFEEngine.SAFE memory _safeInfo = ISAFEEngine(safeEngine).safes(_sData.collateralType, _sData.safeHandler);
  int256 _deltaCollateral = _safeInfo.lockedCollateral.toInt();
  int256 _deltaDebt = _safeInfo.generatedDebt.toInt();
  ISAFEEngine(safeEngine).transferSAFECollateralAndDebt(
    _sData.collateralType, _src, _sData.safeHandler, _deltaCollateral, _deltaDebt
  );
  emit EnterSystem(msg.sender, _src, _safe);
}

Assessed type

Other

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/oracles/CamelotRelayer.sol#L70 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/oracles/CamelotRelayer.sol#L74 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/oracles/CamelotRelayer.sol#L93

Vulnerability details

Impact

The price lookup feature of the CamelotRelayer contract does not work.

Proof of Concept

The CamelotRelayer contract uses Uniswap V3's OracleLibrary to retrieve price information from Camelot pool. However, Camelot pool does not have the exact same interface as Uniswap V3, so the transaction will be reverted

The getResultWithValidity function of the CamelotRelayer contract calls OracleLibrary.getOldestObservationSecondsAgo and OracleLibrary.consult. And read function of the CamelotRelayer contract calls OracleLibrary.consult .

function getResultWithValidity() external view returns (uint256 _result, bool _validity) {
    // If the pool doesn't have enough history return false
@>  if (OracleLibrary.getOldestObservationSecondsAgo(camelotPair) < quotePeriod) {
      return (0, false);
    }
    // Consult the query with a TWAP period of quotePeriod
@>  (int24 _arithmeticMeanTick,) = OracleLibrary.consult(camelotPair, quotePeriod);
    // Calculate the quote amount
    uint256 _quoteAmount = OracleLibrary.getQuoteAtTick({
      tick: _arithmeticMeanTick,
      baseAmount: baseAmount,
      baseToken: baseToken,
      quoteToken: quoteToken
    });
    // Process the quote result to 18 decimal quote
    _result = _parseResult(_quoteAmount);
    _validity = true;
  }

function read() external view returns (uint256 _result) {
    // This call may revert with 'OLD!' if the pool doesn't have enough cardinality or initialized history
@>  (int24 _arithmeticMeanTick,) = OracleLibrary.consult(camelotPair, quotePeriod);
    uint256 _quoteAmount = OracleLibrary.getQuoteAtTick({
      tick: _arithmeticMeanTick,
      baseAmount: baseAmount,
      baseToken: baseToken,
      quoteToken: quoteToken
    });
    _result = _parseResult(_quoteAmount);
  }

This is the OracleLibrary code from Uniswap V3. The getOldestObservationSecondsAgo function calls the pool contract's slot0 , observations . The consult function calls the observe function of pool contract.

function getOldestObservationSecondsAgo(address pool) internal view returns (uint32 secondsAgo) {
@>  (, , uint16 observationIndex, uint16 observationCardinality, , , ) = IUniswapV3Pool(pool).slot0();
    require(observationCardinality > 0, 'NI');

    (uint32 observationTimestamp, , , bool initialized) =
@>      IUniswapV3Pool(pool).observations((observationIndex + 1) % observationCardinality);

    // The next index might not be initialized if the cardinality is in the process of increasing
    // In this case the oldest observation is always in index 0
    if (!initialized) {
@>      (observationTimestamp, , , ) = IUniswapV3Pool(pool).observations(0);
    }

    secondsAgo = uint32(block.timestamp) - observationTimestamp;
}

function consult(address pool, uint32 secondsAgo)
    internal
    view
    returns (int24 arithmeticMeanTick, uint128 harmonicMeanLiquidity)
{
    require(secondsAgo != 0, 'BP');

    uint32[] memory secondsAgos = new uint32[](2);
    secondsAgos[0] = secondsAgo;
    secondsAgos[1] = 0;

    (int56[] memory tickCumulatives, uint160[] memory secondsPerLiquidityCumulativeX128s) =
@>      IUniswapV3Pool(pool).observe(secondsAgos);

    ...
}

https://github.com/Uniswap/v3-periphery/blob/697c2474757ea89fec12a4e6db16a574fe259610/contracts/libraries/OracleLibrary.sol#L74

https://github.com/Uniswap/v3-periphery/blob/697c2474757ea89fec12a4e6db16a574fe259610/contracts/libraries/OracleLibrary.sol#L16

However, the functions slot0, observations, and observe do not exist in Camelot's Pool. I found Camelot's pool through CAMELOT_V3_FACTORY (0x1a3c9B1d2F0529D97f2afC5136Cc23e58f1FD35B) at Registry.s.sol.

If you call poolDeployer view function at CAMELOT_V3_FACTORY, it returns 0x6Dd3FB9653B10e806650F107C3B5A0a6fF974F65, and you can see the Pool code in this contract. https://arbiscan.io/address/0x6dd3fb9653b10e806650f107c3b5a0a6ff974f65#code

Tools Used

Manual Review

Recommended Mitigation Steps

Don’t use Uniswap V3 OracleLibrary for Camelot. You need to implement your own logic to communicate with the Camelot pool. These are functions/variables in Camelot pool that are similar to Uniswap V3 Pool’s.

• slot0 → globalState : https://arbiscan.io/address/0x6dd3fb9653b10e806650f107c3b5a0a6ff974f65#code#F15#L24
• observations → timepoints : https://arbiscan.io/address/0x1a3c9B1d2F0529D97f2afC5136Cc23e58f1FD35B#code#F3#L22
  • DataStorage is deployed at createPool and attached at new pool. https://arbiscan.io/address/0x1a3c9B1d2F0529D97f2afC5136Cc23e58f1FD35B#code#F1#L67
• observe → getTimepoints https://arbiscan.io/address/0x6dd3fb9653b10e806650f107c3b5a0a6ff974f65#code#F3#L176

Assessed type

Other

Judge has assessed an item in Issue #165 as 2 risk. The relevant finding follows:

Clear safeCan in transferSAFEOwnership Links to affected code https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L136

Impact Old approval remains even if user gets SAFE again.

Proof of Concept There is no removal safeCan at transferSAFEOwnership . When the user gets SAFE again, the old approval is still remained.

modifier safeAllowed(uint256 _safe) { address _owner = _safeData[_safe].owner; if (msg.sender != _owner && safeCan[_owner][_safe][msg.sender] == 0) revert SafeNotAllowed(); _; }

function transferSAFEOwnership(uint256 _safe, address _dst) external { require(msg.sender == address(vault721), 'SafeMngr: Only Vault721');

if (_dst == address(0)) revert ZeroAddress();
SAFEData memory _sData = _safeData[_safe];
if (_dst == _sData.owner) revert AlreadySafeOwner();

_usrSafes[_sData.owner].remove(_safe);
_usrSafesPerCollat[_sData.owner][_sData.collateralType].remove(_safe);

_usrSafes[_dst].add(_safe);
_usrSafesPerCollat[_dst][_sData.collateralType].add(_safe);

_safeData[_safe].owner = _dst;

emit TransferSAFEOwnership(msg.sender, _safe, _dst);

} Tools Used Manual Review

Recommended Mitigation Steps Delete safeCan at transferSAFEOwnership . You might use Set instead of mapping.