DYAD - niloy's results

QA Report

Issue 1 - Missing Return Value Handling in deposit Method

Consequences:

The deposit method fails to process the return value from the SafeTransferLib's safeTransferFrom method, which facilitates asset transfers from the caller to the vault.

Example for Clarification:

function deposit(uint id, uint amount) public onlyVaultManager {
  id2asset[id] += amount;
  emit Deposit(id, amount);
}

Mitigation:

Validate the return value from the safeTransferFrom method to ensure the transfer was successful.

function deposit(uint id, uint amount) public onlyVaultManager {
  require(asset.safeTransferFrom(msg.sender, address(this), amount), "Transfer failed");
  id2asset[id] += amount;
  emit Deposit(id, amount);
}

Issue 2 - Absence of Parameter Checks

Consequences:

Critical methods like deposit, withdraw, and move lack checks on the amount parameter, potentially leading to errors or unexpected behaviors if zero or excessively large values are provided.

Example for Clarification:

function deposit(uint id, uint amount) public onlyVaultManager {
  // ...
}

function withdraw(uint id, address to, uint amount) external onlyVaultManager {
  // ...
}

function move(uint from, uint to, uint amount) external onlyVaultManager {
  // ...
}

Mitigation:

Incorporate checks to verify that the amount is within reasonable limits.

function deposit(uint id, uint amount) public onlyVaultManager {
  require(amount > 0, "Deposit amount must be greater than zero");
  // ...
}

function withdraw(uint id, address to, uint amount) external onlyVaultManager {
  require(amount > 0, "Withdrawal amount must be greater than zero");
  require(amount <= id2asset[id], "Insufficient balance");
  // ...
}

function move(uint from, uint to, uint amount) external onlyVaultManager {
  require(amount > 0, "Move amount must be greater than zero");
  // ...
}

Issue 3 - Omission of Event Logging

Consequences:

Key functions like setKeroseneManager in VaultManagerV2 and the constructor in KerosineVault do not log events, reducing the visibility and auditability of contract operations.

Example for Clarification:

function setKeroseneManager(KerosineManager _keroseneManager) external initializer {
  keroseneManager = _keroseneManager;
}

constructor(IVaultManager _vaultManager, ERC20 _asset, KerosineManager _kerosineManager) {
  vaultManager = _vaultManager;
  asset = _asset;
  kerosineManager = _kerosineManager;
}

Mitigation:

Implement event logging for significant function executions and state changes.

event KeroseneManagerSet(address indexed keroseneManager);

function setKeroseneManager(KerosineManager _keroseneManager) external initializer {
  keroseneManager = _keroseneManager;
  emit KeroseneManagerSet(address(_keroseneManager));
}

event VaultInitialized(address indexed vaultManager, address indexed asset, address indexed kerosineManager);

constructor(IVaultManager _vaultManager, ERC20 _asset, KerosineManager _kerosineManager) {
  vaultManager = _vaultManager;
  asset = _asset;
  kerosineManager = _kerosineManager;
  emit VaultInitialized(address(_vaultManager), address(_asset), address(_kerosineManager));
}

Issue 4 - Risk of Division by Zero

Consequences:

Functions collatRatio in VaultManagerV2 and assetPrice in UnboundedKerosineVault execute divisions without safeguards against division by zero, risking transaction reversion or erratic outcomes.

Example for Clarification:

function collatRatio(uint id) public view returns (uint) {
  uint _dyad = dyad.mintedDyad(address(this), id);
  if (_dyad == 0) return type(uint).max;
  return getTotalUsdValue(id).divWadDown(_dyad);
}

function assetPrice() public view override returns (uint) {
  // ...
  uint denominator = kerosineDenominator.denominator();
  return numerator * 1e8 / denominator;
}

Mitigation:

Implement pre-checks to confirm non-zero divisors before performing division operations.

function collatRatio(uint id) public view returns (uint) {
  uint _dyad = dyad.mintedDyad(address(this

), id);
  require(_dyad != 0, "No minted Dyad for the given ID");
  return getTotalUsdValue(id).divWadDown(_dyad);
}

function assetPrice() public view override returns (uint) {
  // ...
  uint denominator = kerosineDenominator.denominator();
  require(denominator != 0, "Denominator cannot be zero");
  return numerator * 1e8 / denominator;
}

Issue 5 - Insufficient Access Restrictions

Consequences:

The setDenominator function in UnboundedKerosineVault permits the contract owner to modify the kerosineDenominator contract address without adequate access controls, opening the possibility for unauthorized changes.

Example for Clarification:

function setDenominator(KerosineDenominator _kerosineDenominator) external onlyOwner {
  kerosineDenominator = _kerosineDenominator;
}

Mitigation:

Adopt robust access control frameworks, such as role-based access control systems, to ensure only authorized parties can modify critical settings.

Issue 6 - Inadequate Documentation

Consequences:

The contracts lack comprehensive NatSpec comments, complicating the understanding and audit processes for developers and external auditors.

Mitigation:

Enhance contract documentation with complete NatSpec annotations, detailing the purpose, operations, parameters, and return values of each function.

Issue 7 - Uncontrolled Arithmetic Operations

Consequences:

The assetPrice function in UnboundedKerosineVault conducts arithmetic operations without safeguards against overflows or underflows, posing risks of incorrect computations or unexpected behavior.

Example for Clarification:

function assetPrice() public view override returns (uint) {
  // ...
  uint numerator = tvl - dyad.totalSupply();
  uint denominator = kerosineDenominator.denominator();
  return numerator * 1e8 / denominator;
}

Mitigation:

Employ libraries like SafeMath to conduct arithmetic operations securely, or manually verify conditions to prevent arithmetic errors.

function assetPrice() public view override returns (uint) {
  // ...
  uint numerator = tvl - dyad.totalSupply();
  uint denominator = kerosineDenominator.denominator();
  require(denominator != 0, "Denominator cannot be zero");
  require(numerator <= type(uint256).max / 1e8, "Numerator too large");
  return numerator * 1e8 / denominator;
}

Issue 8 - Missing Reentrancy Guard

Consequences:

The withdraw function in UnboundedKerosineVault allows asset transfers to external addresses without reentrancy protection, potentially exposing the function to exploitation through reentrancy attacks.

Example for Clarification:

function withdraw(uint id, address to, uint amount) external onlyVaultManager {
  id2asset[id] -= amount;
  asset.safeTransfer(to, amount); 
  emit Withdraw(id, to, amount);
}

Mitigation:

Implement reentrancy guards to shield against potential reentrancy threats, using patterns such as checks-effects-interactions or reentrancy modifiers.

function withdraw(uint id, address to, uint amount) external onlyVaultManager nonReentrant {
  id2asset[id] -= amount;
  asset.safeTransfer(to, amount); 
  emit Withdraw(id, to, amount);
}