Open Dollar - josephdara's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L105-L109

Vulnerability details

Impact

In the codebase and in various standards in smart contract implementation, users can give other addresses allowances to manage their assets and protocol participation. However, there are limits to this. One important limit is allowing an address that was given allowance, give the same allowance to other addresses. For erc20s, only the token owner can approve another address to spend their tokens, It is the same with ERC721 approve and setApprovalForAll.

However in this contract, once a user approves another address using allowSAFE(), they can never truly revoke the permission for that user because that approved address can give infinite address approval on that singular safe before the permission is revoked.

Proof of Concept

The root cause is the function and it's modifier

  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);
  }


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

Here in the modifier, we see that a user who has allowance on an address's SAFE can immediately give approvals to other addresses since safeCan[_owner][_safe][msg.sender] != 0. allowSAFE() should verify that the msg.sender is the safe Owner.

Users sign bad transactions alot of times due to phishing attacks or bad links, but at least tokens can curb this using a remove approval method on that singular address. With the possibility of each address giving the allowance to another address which can be given to another address while all of them still maintaining the allowance, a singular mistake can cause a lot of issues.

By calling removeSAFE() they can DOS transferring whole vault to another address They can also modify the safe, quit the system, enter the system and many other actions that harms the user.

Tools Used

Manual Review

Recommended Mitigation Steps

allowHandler() has the right implementation for giving allowance. Replace the current allowSAFE() function with

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

Assessed type

Other

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L10 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L70-L81

Vulnerability details

Impact

The camelot relayer uses the uniV3 OracleLibrary to fetch data from uniswap however there is an issue here. The functionality of camelot and uniswap v3 are different, so are the function selectors being used in the library. Therefore calls made to camelot would revert.

Proof of Concept

The function getResultWithValidity() makes multiple calls using the oracle library:

  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;
  }

Examining each call:

 if (OracleLibrary.getOldestObservationSecondsAgo(camelotPair) < quotePeriod) {
      return (0, false);
    }

in the oracle library, makes

• IUniswapV3Pool(pool).slot0()
• IUniswapV3Pool(pool).observations() Both this functions are unavailable on the camelot pair contract. The Camelot AMMV3 uses the Algebra Contracts, Looking at one of the live contracts for DAI/WETH available at https://arbiscan.io/address/0x6c0790a175dbce981301db099ef713794f41924a#code We see the differences, instead of slot0() we have:

  struct GlobalState {
    uint160 price; // The square root of the current price in Q64.96 format
    int24 tick; // The current tick
    uint16 feeZto; // The current fee for ZtO swap in hundredths of a bip, i.e. 1e-6
    uint16 feeOtz; // The current fee for OtZ swap in hundredths of a bip, i.e. 1e-6
    uint16 timepointIndex; // The index of the last written timepoint
    uint8 communityFeeToken0; // The community fee represented as a percent of all collected fee in thousandths (1e-3)
    uint8 communityFeeToken1;
    bool unlocked; // True if the contract is unlocked, otherwise - false
  }


  GlobalState public override globalState;

The observations() function also is none existent.

In the function getResultWithValidity() , the Oracle library is called with consult() which calls observe on the pair contract:

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

However this function does not exist on the camelot pair contract, instead we have

  function getTimepoints(uint32[] calldata secondsAgos)

Tools Used

Manual Review, Camelot AMM V3 docs and contracts, Uniswap V3 Oracle Library and V3 Pool contract

Recommended Mitigation Steps

Implement the proper function list that corresponds with the Camelot V3 interface

Assessed type

Oracle