Open Dollar - tnquanghuy0512's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/AccountingEngine.sol#L213-L221

Vulnerability details

Impact

In the Math library, 1 WAD is equal to 1e18 which is assumed 1 For example: wmul(2e18, 4e18) = 8e18

In AccountingEngine contract, the devs assumed that 1 WAD is equal to 0.01 (1%), 100 WAD is equal to 1 (100%). Hence, the function auctionSurplus() will make start the auction with selling amount way bigger than expected. Moreover, if the selling amount is bigger than the balance of AccountingEngine contract, it will cause DOS

Proof of Concept

  function auctionSurplus() external returns (uint256 _id) {
    ...

    // auction surplus percentage
    if (_params.surplusTransferPercentage < ONE_HUNDRED_WAD) {
      _id = surplusAuctionHouse.startAuction({
@@>   _amountToSell: _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage),
        _initialBid: 0
      });

      lastSurplusTime = block.timestamp;
      emit AuctionSurplus(_id, 0, _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage));
    }

    ...
  }

We have the table of scenario:

Tools Used

Manual review

Recommended Mitigation Steps

Change this in ACcountingEngine.auctionSurplus()

-    if (_params.surplusTransferPercentage < ONE_HUNDRED_WAD) {
+    if (_params.surplusTransferPercentage < WAD) {

      _id = surplusAuctionHouse.startAuction({
-       _amountToSell: _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage),
+       _amountToSell: _params.surplusAmount.wmul(WAD - _params.surplusTransferPercentage),

        _initialBid: 0
      });

      lastSurplusTime = block.timestamp;
-     emit AuctionSurplus(_id, 0, _params.surplusAmount.wmul(ONE_HUNDRED_WAD 
- _params.surplusTransferPercentage));

+     emit AuctionSurplus(_id, 0, _params.surplusAmount.wmul(WAD 
- _params.surplusTransferPercentage));
    }

Assessed type

Math

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L58 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/UniV3Relayer.sol#L64

Vulnerability details

Impact

It's quite often that ERC20s have decimals higher than 18. Because of that, constructor in CamelotRelayer and UniV3Relayer will get reverted with quote token that has high decimals because of underflow in this line multiplier = 18 - IERC20Metadata(_quoteToken).decimals();

Tools Used

Manual Review

Recommended Mitigation Steps

Make multiplier variable has int256 type.

Assessed type

ERC20

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/SAFEHandler.sol#L1-L19 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L112-L115 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L59-L62

Vulnerability details

Impact

SafeHandler contract doesn't have any method to call to ODSafeManager.allowHandler(). Because of that, functions in ODSafeManager contract which use handlerAllowed modifier will never be called successfully. There're two function quitSystem() and enterSystem() that use handlerAllowed modifier.

Proof of Concept

This is the whole SAFEHandler contract, it doesn't have any method that call to ODSafeManager.allowHandler()

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.19;

import {ISAFEEngine} from '@interfaces/ISAFEEngine.sol';

contract SAFEHandler {
  constructor(address _safeEngine) {
    ISAFEEngine(_safeEngine).approveSAFEModification(msg.sender);
  }
}

Hence, it can't call to allowHandler(), which give users permission to execute action within the safe

  function allowHandler(address _usr, uint256 _ok) external {
    handlerCan[msg.sender][_usr] = _ok;
    emit AllowHandler(msg.sender, _usr, _ok);
  }

Because of that, no one can surpass the handlerAllowed modifier with valid handler address:

  modifier handlerAllowed(address _handler) {
    if (msg.sender != _handler && handlerCan[_handler][msg.sender] == 0) revert HandlerNotAllowed();
    _;
  }

enterSystem() and quitSystem() use handlerAllowed modifier:

function enterSystem(address _src, uint256 _safe) external handlerAllowed(_src) safeAllowed(_safe)
function quitSystem(uint256 _safe, address _dst) external safeAllowed(_safe) handlerAllowed(_dst)

Tools Used

Manual Review

Recommended Mitigation Steps

I think of two way to achieve this:

1. Add a function in SafeHandler contract that can call to ODSafeManager.allowHandler()
2. Add an immutable variable SAFE_ID in SafeHandler contract and change this to ODSafeManager.allowHandler():

-  function allowHandler(uint256 _safeId, address _usr, uint256 _ok) external {
+  function allowHandler(address _usr, uint256 _ok) external {

+   SAFEData memory _sData = _safeData[_safeId];
+   require(_sData.owner == msg.sender);

-   handlerCan[msg.sender][_usr] = _ok;
+   handlerCan[_sData.safeHandler][_usr] = _ok;

-   emit AllowHandler(msg.sender, _usr, _ok);
  }

Assessed type

Other

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/gov/ODGovernor.sol#L41

Vulnerability details

Impact

There's two thing that we need to resolve here:

1. Governor's initial votingDelay and votingPeriod is too short (1 block and 15 blocks accordingly), making malicious user can manipulate easily
2. Inconsistent block.number in Arbitrum, with votingDelay votingPeriod is calculated by block.number as a clock, it even easier for malicious to manipulate.

Similar issue: https://github.com/code-423n4/2023-06-lybra-findings/issues/114

Proof of Concept

VotingDelay definition: Delay, between the proposal is created and the vote starts

VotingPeriod definition: Delay between the vote start and vote end

Tools Used

Manual Review

Recommended Mitigation Steps

Make the initial votingDelay and votingPeriod longer to resolve issue 1 Use block.timestamp rather than block.number to resolve issue 2

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

In order for other user to call ODSafeManager.modifySAFECollateralization() ODSafeManager.transferCollateral() ODSafeManager.transferInternalCoins() ODSafeManager.quitSystem() ODSafeManager.enterSystem() ODSafeManager.moveSAFE() ODSafeManager.removeSAFE(), the safe owner has to set the their permission to owner's safe by call ODSafeManager.allowSAFE().

The problem here is that anyone after getting their permission to the safe can add/remove others permission in that safe.

Moreover, the owner basically can't revoke all the permissions if the owner is not a contract (which is highly the case), since revoking permission using transferSAFEOwnership() can only revoke once per transaction. If the owner try to give permission to multicall contract in order to revoke all the malicious permission, malicious users can just simply remove multicall contract's permission in between the two transaction

Proof of Concept

allowSAFE() can be called by allowed user to remove/add others user:

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

Function that can be called by allowed user (functions that have safeAllowed modifier):

function modifySAFECollateralization(uint256 _safe, int256 _deltaCollateral, int256 _deltaDebt) external safeAllowed(_safe)
function transferCollateral(uint256 _safe, address _dst, uint256 _wad) external safeAllowed(_safe)
function transferCollateral(bytes32 _cType, uint256 _safe, address _dst, uint256 _wad) external safeAllowed(_safe)
function transferInternalCoins(uint256 _safe, address _dst, uint256 _rad) external safeAllowed(_safe)
function quitSystem(uint256 _safe, address _dst) external safeAllowed(_safe) handlerAllowed(_dst)
function enterSystem(address _src, uint256 _safe) external handlerAllowed(_src) safeAllowed(_safe)
function moveSAFE(uint256 _safeSrc, uint256 _safeDst) external safeAllowed(_safeSrc) safeAllowed(_safeDst)
function removeSAFE(uint256 _safe) external safeAllowed(_safe)
function protectSAFE(uint256 _safe, address _liquidationEngine, address _saviour) external safeAllowed(_safe)

Tools Used

Manual Review

Recommended Mitigation Steps

Make the function allowSAFE() can only be called by safe owner

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

Assessed type

Access Control

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 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L70-L76 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L93-L94

Vulnerability details

Impact

CamelotRelayer contract's purpose is using CamelotV3 protocol as a price oracle. But CamelotRelayer treated Camelot protocol as similar as UniswapV3 protocol. CamelotV3 us based from Algebra v1.9 which is not quite alike as UniswapV3. Because of this, CamelotRelayer contract will not useable.

Proof of Concept

From Camelot docs: Camelot's V3 AMM is based on Algebra's V1.9

In CamelotRelayer contract, it uses UniswapV3's OracleLibrary

import {OracleLibrary} from '@uniswap/v3-periphery/contracts/libraries/OracleLibrary.sol';

It uses 3 functions in UniswapV3's OracleLibrary:

• OracleLibrary.getOldestObservationSecondsAgo()
• OracleLibrary.consult()
• OracleLibrary.getQuoteAtTick()

Those 3 functions in OracleLibrary call to 3 function in UniwapV3Pool:

• IUniswapV3Pool.slot0()
• IUniswapV3Pool.observations()
• IUniswapV3Pool.observe()

This is the USDT-USDC CamelotPair contract's read functions, notice here that there's no slot0(), observations(), observe() read function. Hence, CamelotRelayer.getResultWithValidity() and CamelotRelayer.read() will forever DOS

Tools Used

Manual Review

Recommended Mitigation Steps

The devs need to read carefully Algebra's docs to fix this

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

ODSafeManager.transferSAFEOwnership() function delete safeCan of the previous owner. Because of that, when one of the old owner owns the NFTs again, there can be a risk of now-malicious old permission that can execute actions to the safe.

Proof of Concept

Imagine this scenario:

• User A have a Safe/NFT X
• User A give the safe permission of NFT X using allowSAFE() to user B to do something
• User A now sell the NFT X, user C now is the owner of the NFT X
• After some months, A buys NFT X from the marketplace
• Because A is now the owner of the NFT X, user B now has the permission to the NFT X
• But user B is now not trusted by user A, B maliciously executes actions to the NFT X

  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;

    <@@ NOTICE here that's the function doesn't `delete safeCan[_sData.owner][_safe]` here

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

Tools Used

Manual Review

Recommended Mitigation Steps

  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;

+   delete safeCan[_sData.owner][_safe];

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

Assessed type

Other