Open Dollar - JCK's results

Gas Optimizations

[G-01] the value of debtQueue[_debtBlockTimestamp] is already cached use _debtBlock instead of using debtQueue[_debtBlockTimestamp]

file: src/contracts/AccountingEngine.sol

123     function popDebtFromQueue(uint256 _debtBlockTimestamp) external {
    if (block.timestamp < _debtBlockTimestamp + _params.popDebtDelay) revert AccEng_PopDebtCooldown();

    uint256 _debtBlock = debtQueue[_debtBlockTimestamp];

    if (_debtBlock == 0) revert AccEng_NullAmount();

    totalQueuedDebt = totalQueuedDebt - _debtBlock;
    debtQueue[_debtBlockTimestamp] = 0;

    emit PopDebtFromQueue(_debtBlockTimestamp, _debtBlock);
  }

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L126

[G‑02] Counting down in for statements is more gas efficient

Counting down is more gas efficient than counting up because neither we are making zero variable to non-zero variable and also we will get gas refund in the last transaction when making non-zero to zero variable.

by changing this logic you can save 12171 gas per one for loop

Tools used Remix

file:  src/contracts/proxies/ODSafeManager.sol

91      for (uint256 _i; _i < _safes.length; _i++) {

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

Test Code

contract GasTest is DSTest {
    Contract0 c0;
    Contract1 c1;
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    function testGas() public {
        c0.AddNum();
        c1.AddNum();
    }
}
contract Contract0 {
    uint256 num = 3;
    function AddNum() public {
        uint256 _num = num;
        for(uint i=0;i<=9;i++){
            _num = _num +1;
        }
        num = _num;
    }
}
contract Contract1 {
    uint256 num = 3;
    function AddNum() public {
        uint256 _num = num;
        for(uint i=9;i>=0;i--){
            _num = _num +1;
        }
        num = _num;
    }
}

[G-03] Don’t cache calls that are only used once

file: src/contracts/proxies/ODSafeManager.sol

122   address _safeHandler = address(new SAFEHandler(safeEngine));

208   int256 _deltaCollateral = _safeInfo.lockedCollateral.toInt();

209   int256 _deltaDebt = _safeInfo.generatedDebt.toInt();

222   208   int256 _deltaCollateral = _safeInfo.lockedCollateral.toInt();

223   int256 _deltaDebt = _safeInfo.generatedDebt.toInt();

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

file: src/contracts/proxies/Vault721.sol

110   address _safeManager = address(safeManager);

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/Vault721.sol#L110

file: src/contracts/proxies/actions/BasicActions.sol

60   uint256 _coinAmount = ISAFEEngine(_safeEngine).coinBalance(_safeHandler);

78   uint256 _rate = ISAFEEngine(_safeEngine).cData(_cType).accumulatedRate;

79   uint256 _generatedDebt = ISAFEEngine(_safeEngine).safes(_cType, _safeHandler).generatedDebt;

80   uint256 _coinAmount = ISAFEEngine(_safeEngine).coinBalance(_usr);

101  address _safeEngine = ODSafeManager(_manager).safeEngine();

128  address _safeEngine = ODSafeManager(_manager).safeEngine();

179  address _safeEngine = ODSafeManager(_manager).safeEngine();

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/actions/BasicActions.sol#l60

[G-04] public functions not called by the contract should be declared external instead

when a function is declared as public, it is generated with an internal and an external interface. This means the function can be called both internally (within the contract) and externally (by other contracts or accounts). However, if a public function is never called internally and is only expected to be invoked externally, it is more gas-efficient to explicitly declare it as external.

file: src/contracts/proxies/Vault721.sol

140   function tokenURI(uint256 _safeId) public view override returns (string memory uri) {

147   function contractURI() public view returns (string memory uri) {

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/Vault721.sol#L140

[G-05] Multiple accesses of a mapping/array should use a storage pointer

Caching a mapping’s value in a storage pointer when the value is accessed multiple times saves ~40 gas per access due to not having to perform the same offset calculation every time. Help the Optimizer by saving a storage variable’s reference instead of repeatedly fetching it.

To achieve this, declare a storage pointer for the variable and use it instead of repeatedly fetching the reference in a map or an array. As an example, instead of repeatedly calling _pumps[i], save its reference via a storage pointer: _pumpsInfo storage pumpsInfo = _pumps[i] and use the pointer instead.

Cache storage pointers for debtQueue[_debtBlockTimestamp];

file: src/contracts/AccountingEngine.sol

126   uint256 _debtBlock = debtQueue[_debtBlockTimestamp];

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L126

Cache storage pointers for _safeData[_safes[_i]];

file:  src/contracts/proxies/ODSafeManager.sol

92   _safeHandlers[_i] = _safeData[_safes[_i]].safeHandler;

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

file:  src/contracts/proxies/Vault721.sol

96    require(_proxyRegistry[_proxy] != address(0), 'V721: non-native proxy');

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/Vault721.sol#L96

[G-06] Do not initialize variables with default values

file:  src/contracts/AccountingEngine.sol

248   totalQueuedDebt = 0;

249   totalOnAuctionDebt = 0;

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L248

file: src/contracts/oracles/CamelotRelayer.sol

84  _validity = true;

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

file:  src/contracts/oracles/UniV3Relayer.sol

90    _validity = true;

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/oracles/UniV3Relayer.sol#L90

[G-07] State variables which are not modified within functions should be set as constant or immutable for values set at deployment

Cache such variables and perform operations on them, if operations include modifications to the state variable(s) then remember to equate the state variable to it’s cached counterpart at the end

file: src/contracts/oracles/CamelotRelayer.sol

31     string public symbol;

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

file: src/contracts/oracles/UniV3Relayer.sol
 
30   string public symbol;

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/oracles/UniV3Relayer.sol#L30

file: src/contracts/proxies/Vault721.sol

18   address public governor;

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/Vault721.sol#L18

[G-08] Use assembly to perform efficient back-to-back calls

If similar external calls are performed back-to-back, we can use assembly to reuse any function signatures and function parameters that stay the same. In addition, we can also reuse the same memory space for each function call (scratch space + free memory pointer), which can potentially allow us to avoid memory expansion costs. In this case, we are also able to efficiently store the function signatures together in memory as one word, saving multiple MLOADs in the process.

Note: In order to do this optimization safely we will cache the free memory pointer value and restore it once we are done with our function calls. We will also set the zero slot back to 0 if neccessary.

Use for this ISAFEEngine(_safeEngine) back to back external call assembly

file:  src/contracts/proxies/actions/BasicActions.sol

78    uint256 _rate = ISAFEEngine(_safeEngine).cData(_cType).accumulatedRate;
      uint256 _generatedDebt = ISAFEEngine(_safeEngine).safes(_cType, _safeHandler).generatedDebt;
      uint256 _coinAmount = ISAFEEngine(_safeEngine).coinBalance(_usr);

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/actions/BasicActions.sol#L78-L80

Use for this ODSafeManager(_manager back to back external call assembly

file:  src/contracts/proxies/actions/BasicActions.sol

382    address _safeEngine = ODSafeManager(_manager).safeEngine();
       ODSafeManager.SAFEData memory _safeInfo = ODSafeManager(_manager).safeData(_safeId);

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/actions/BasicActions.sol#L382-L383

[G-09] Use assembly to write address storage values

file: src/contracts/AccountingEngine.sol    

98   _params = _accEngineParams;

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L98

file:  src/contracts/proxies/Vault721.sol

164   _proxyRegistry[_proxy] = _user;

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/Vault721.sol#L164

[G-10] Use hardcoded address instead of address(this)

Instead of using address(this), it is more gas-efficient to pre-calculate and use the hardcoded address. Foundry’s script.sol and solmate’s LibRlp.sol contracts can help achieve this. Refrences

file:  src/contracts/AccountingEngine.sol

105    return _unqueuedUnauctionedDebt(safeEngine.debtBalance(address(this)));

140    _settleDebt(safeEngine.coinBalance(address(this)), safeEngine.debtBalance(address(this)), _rad);

178    uint256 _coinBalance = safeEngine.coinBalance(address(this));

179    uint256 _debtBalance = safeEngine.debtBalance(address(this));

204    uint256 _coinBalance = safeEngine.coinBalance(address(this));

205    uint256 _debtBalance = safeEngine.debtBalance(address(this));

228    _source: address(this),

255    uint256 _debtToSettle = Math.min(safeEngine.coinBalance(address(this)), safeEngine.debtBalance(address(this)));

264    uint256 _coinBalance = safeEngine.coinBalance(address(this));

265    uint256 _debtBalance = safeEngine.debtBalance(address(this));

271    _source: address(this),

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L105

file:  src/contracts/proxies/actions/BasicActions.sol

203   _transferInternalCoins(_manager, _safeId, address(this), _deltaWad * RAY);

218   _transferCollateral(_manager, _safeId, address(this), _deltaWad);

297    address(this),

298   _getRepaidDebt(_safeEngine, address(this), _safeInfo.collateralType, _safeInfo.safeHandler)

305   _collateralSource: address(this),

306   _debtDestination: address(this),

337   _safe = _openSAFE(_manager, _cType, address(this));

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/actions/BasicActions.sol#L203

[G-11] Cache state variables outside of loop to avoid reading storage on every iteration

Reading from storage should always try to be avoided within loops. In the following instances, we are able to cache state variables outside of the loop to save a Gwarmaccess (100 gas) per loop iteration.

Cache _safeData outside of the loop to save 1 SLOAD per iteration

file:  src/contracts/proxies/ODSafeManager.sol

91   for (uint256 _i; _i < _safes.length; _i++) {
      _safeHandlers[_i] = _safeData[_safes[_i]].safeHandler;
      _cTypes[_i] = _safeData[_safes[_i]].collateralType;
    }

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

[G-12] Use uint256(1)/uint256(2) instead of true/false to save gas for changes

file: src/contracts/oracles/CamelotRelayer.sol

84  _validity = true;

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

file:  src/contracts/oracles/UniV3Relayer.sol

90    _validity = true;

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/oracles/UniV3Relayer.sol#L90

[G-14] Save loop calls

Instead of calling safeData[_safes[_i]] 2 times in each loop for fetching data, it can be saved as a variable.

file: src/contracts/proxies/ODSafeManager.sol

91   for (uint256 _i; _i < _safes.length; _i++) {
      _safeHandlers[_i] = _safeData[_safes[_i]].safeHandler;
      _cTypes[_i] = _safeData[_safes[_i]].collateralType;
    }

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

Phase 1: Documentation and Video Review

A: Start with a comprehensive review of all documentation related to the Open Dollar platform, including the whitepaper, API documentation, developer guides, user guides, and any other available resources.

B: Watch any available walkthrough videos to gain a holistic understanding of the system. Pay close attention to any details about the platform’s architecture, operation, and possible edge cases.

C: Note down any potential areas of concern or unclear aspects for further investigation.

Phase 2: Manual Code Inspection

I: Once familiarized with the operation of the platform, start the process of manual code inspection.

III: Look for common vulnerabilities such as reentrancy, integer overflow/underflow, improper error handling, etc., while also ensuring the code conforms to best programming practices.

IV: Document any concerns or potential issues identified during this stage.

Phase 3: Architecture recommendations:

1. ODGovernor: The codebase follows a modular and composability approach, integrating various OpenZeppelin governance contracts and using inheritance to combine their functionalities. It inherits from several parent contracts, which are widely recognized and well-audited in the Ethereum community, making the codebase more reliable.

2. Vault721: The code is designed to restrict the owner to the Vault721 contract. This is an appropriate and secure approach if the intention is to prevent ownership changes and maintain control within a specific contract.

3. SAFEHandler The code implements a straightforward approach where a new SAFEHandler contract is deployed for each safe. This allows for a unique identifier for each safe. It's a minimalistic and efficient approach for its intended purpose.

4. ODSafeManager: The architecture is designed to manage SAFEs and their permissions within the Open Dollar protocol. The contract efficiently manages SAFEs by utilizing an auto-incremental _safeId to uniquely identify each SAFE. It uses separate mappings for different types of permissions to ensure precise control over who can perform what actions.

Phase 4: Codebase quality analysis:

1. AccountingEngine The code uses various libraries for common functions, which is a good practice to keep the code modular and maintainable. It implements authorization, allowing specific accounts to perform certain actions. It provides a way to set parameters and initialize the contract with specific settings. The contract has functions for debt queue management, debt settlement, surplus auctions, and more, which are well-organized.

2. CamelotRelayer: The code appears well-structured and organized. It clearly defines the base and quote tokens, the Camelot pair, and other necessary parameters. The use of libraries like OracleLibrary for price calculations is a good practice as it keeps the code modular and reduces complexity. The code parses the result into an 18-decimal format for standardization. Error handling is in place to handle situations where the pool doesn't have sufficient history. The code is relatively concise and easy to follow.

3. UniV3Relayer: The code is well-structured and organized. It clearly defines the base and quote tokens, the Uniswap V3 pool, and other necessary parameters. The use of libraries like OracleLibrary for price calculations is a good practice as it keeps the code modular and reduces complexity. Error handling is in place to handle situations where the pool doesn't have sufficient history.

4. ODGovernor: It specifies the token (ODG - Open Dollar Governance token) and the timelock controller as constructor parameters. The use of well-established contracts, such as TimelockController, Governor, and others, demonstrates a high level of code quality and security. The code includes required override functions to customize parameters based on the needs of the OD protocol.

5. ODProxy: The code is simple and focused on its primary purpose: allowing the owner (defined in the constructor) to execute arbitrary calls on a target address. It uses error handling through custom error messages, which is a good practice for providing clear information about why a transaction might revert. The onlyOwner modifier ensures that only the pre-defined owner can call the execute function.

6. Vault721: The code uses modifiers for access control (e.g., onlyGovernor, nonZero) to ensure proper permissions and input validation. The initializeManager and initializeRenderer functions help set the initial instances of safeManager and nftRenderer contract addresses. Events are emitted to provide transparency regarding proxy creation and other important contract actions. Functions like mint, updateNftRenderer, and updateContractURI have restricted access through the onlyGovernor modifier.

7. SAFEHandler: It correctly interacts with the ISAFEEngine contract by calling the approveSAFEModification function to grant the msg.sender (most likely the ODSafeManager) permission to modify the safe. There are no redundant or unnecessary operations, and it effectively achieves its goal.

8. ODSafeManager: The usage of libraries, such as Math and EnumerableSet, is a good practice for code reuse and gas efficiency. The contract interacts correctly with the SAFEEngine and Vault721 contracts. Proper error handling is implemented through the use of custom error types. The use of modifiers (safeAllowed and handlerAllowed) enforces access control. Events are emitted to provide transparency and enable external systems to track changes.

Phase 5: Centralization risks:

1. CamelotRelayer:
The code relies on the Camelot pool for price data. The centralization risks might come from the Camelot pool itself, and it's essential to evaluate the reliability and decentralization of the pool and its data sources.

2. UniV3Relayer:
The code relies on the Uniswap V3 pool for price data. The centralization risks might come from the Uniswap V3 pool itself, and it's essential to evaluate the reliability and decentralization of the pool and its data sources.

3. ODGovernor:
Centralization risks for this code are likely tied to the ownership and control of the governance token and the timelock controller. Careful consideration should be given to who has control over these entities.

4. ODProxy:
The centralization risk in this contract is by design: it enforces that only the specified owner can perform actions. This is a specific security measure and might be suitable for certain use cases, like managing a critical operation within the Vault721 contract. However, it's important to be aware of the limitations and implications of centralization in a broader context.

5. Vault721:
The contract assigns a governor address, and most critical functions are restricted to this governor. This introduces centralization risk if the governor's private key is compromised or if there is a lack of transparency regarding the governor's identity.
Centralization risk is also introduced by design when the contract deploys and manages proxies.

6. ODSafeManager:
The centralization risks are relatively low within this contract. It is primarily designed to interact with other contracts, and its permissions are well-defined.
The ownership of the SAFEs is transferred via the Vault721 contract. Therefore, the centralization risk might be more related to how Vault721 is governed.

Phase 6 Mechanism Review:

1. AccountingEngine:
The code implements functions for managing debt queues, settling debt, and conducting surplus auctions.
The contract seems to handle these mechanisms reasonably well, but the effectiveness of these mechanisms depends on the overall design and functionality of the entire system.

2. CamelotRelayer:
The code effectively fetches the price from the Camelot pool and provides methods to get the result with validity.
It uses the time-weighted average price (TWAP) for better accuracy.

3. ODGovernor:
The contract allows for proposing and executing changes to the protocol. Proposals can be created with specific targets, values, and calldata, and the proposal must pass a voting process based on the set parameters (voting delay, voting period, quorum, etc.).
The use of a timelock period between proposal success and execution adds an additional layer of security to prevent rushed decisions.

4.SAFEHandler:
The core mechanism is straightforward. Upon deployment, this contract grants specific permissions to the ODSafeManager. The specific mechanism used (approveSAFEModification) depends on the implementation of the ISAFEEngine contract.\

5. ODSafeManager:
The core mechanism of this contract is to manage SAFEs and their permissions efficiently.
It allows users to open SAFEs, transfer ownership, modify collateralization, transfer collateral and internal coins, and more. These functions are necessary for a SAFE management system.

Phase 7: Systemic Risks:

Systemic risks are typically associated with governance. If the governance process is centralized or controlled by a small group, it can lead to vulnerabilities and systemic risks for the entire protocol. It is essential to ensure a decentralized and well-distributed governance process.

Phase 9: Time Spent

15:30 hours

Time spent:

15.5 hours