final_audit_report.md

final_audit_report.md

SukukFI Final Audit Report: Resolutions & Justifications

This document provides a comprehensive, itemized report of all findings identified in the C4 audit report (https://gist.github.com/itsmetechjay/15522c3a7102c6526f2c6711bacfabd6/) and details how each was addressed, either through an implemented code fix or an architectural justification for rejection.

---

High Risk Findings

[H-01] Missing Authorization Check Allows Unauthorized Fund Withdrawal

• Status: Skipped (Already Fixed)
• Explanation: This vulnerability had already been identified and resolved by the sponsor in the underlying repository. In WERC7575Vault.sol, the caller identity is strictly checked against the owner variable, and _shareToken.spendAllowance(owner, msg.sender, shares) correctly decrements allowance to prevent unauthorized withdrawals.

---

Medium Risk Findings

[M-01] The unregistering of vaults can be DoSed by a malicious user.

• Status: Fixed
• Resolution: Removed the strict balanceOf(vaultAddress) != 0 check in unregisterVault() in both ShareTokenUpgradeable.sol and WERC7575ShareToken.sol. This prevents DoS attacks where a malicious user transfers dust to the vault before unregistration. The vault can now be safely unregistered as long as all pending and claimable records are zero.

[M-02] Stale Redemption Liabilities Lead to Leveraged Losses for Remaining Shareholders and Potential Denial of Service

• Status: Rejected (Invalid Finding)
• Justification: The auditor proposed that committed assets should not remain in the "investable" volatile pool. However, the SukukFI codebase already strictly segregates fulfilled assets from the investable pool, preventing any leveraged loss mathematically:
  1. Physical Fulfillment: ERC7575VaultUpgradeable::fulfillRedeem(assets) reverts if assets > totalAssets(). The manager cannot fulfill a redemption unless those volatile assets have already physically been converted to plain stable cash in the vault's balance.
  2. Strict Segregation: Inside totalAssets(), the vault subtracts $.totalClaimableRedeemAssets from the balance.
  3. Reinvestments Blocked: investAssets(amount) checks amount > totalAssets(). Because totalAssets() statically excludes the committed funds, the manager is physically and mathematically prevented from taking the fulfilled user's segregated cash and risking it back into the market. Therefore, the fulfilled user does not share in future market losses because their cash is no longer exposed to the market.

[M-03] fulfillRedeem may corrupt share price and allow over-redemption of assets

• Status: Fixed
• Resolution: Added an explicit safety bounds check (if (assets > totalAssets()) revert...) in ERC7575VaultUpgradeable.sol#fulfillRedeem() to ensure assets <= totalAssets(). This prevents the manager from accidentally inflating the share price (PPS) through invalid or over-funded redemptions.

---

Low Risk and Informational Issues

[L-01] Vault approval is not revoked in ShareTokenUpgradeable::unregisterVault() flow

• Status: Fixed
• Resolution: In ShareTokenUpgradeable.sol::unregisterVault(), added logic to securely revoke the investment ShareToken's ERC20 allowance upon vault unregistration (IERC20($.investmentShareToken).approve(vaultAddress, 0)).

[L-02] ShareTokenUpgradeable and WERC7575ShareToken are non-compliant with expected ERC-7575 Standard due to incorrect supportsInterface() implementation

• Status: Fixed
• Resolution: Updated both ShareTokenUpgradeable.sol and WERC7575ShareToken.sol to correctly include the IERC7575Share interface ID (0xf815c03d) in their supportsInterface definitions.

[L-03] WERC7575ShareToken::batchTransfers() / WERC7575ShareToken::rBatchTransfers() is missing pause check

• Status: Rejected (Invalid Finding)
• Justification: This recommendation demonstrates a misunderstanding of the protocol's access control architecture. The whenNotPaused modifier halts non-privileged users during an emergency. Conversely, batchTransfers and rBatchTransfers are strictly permissioned administrative tools (onlyValidator). If the protocol is paused due to an accounting error, the validators actually require the ability to run these settlement functions to resolve the balances. Restricting them with whenNotPaused would intentionally brick the protocol's emergency resolution mechanism.

[L-04] ERC7575VaultUpgradeable should use ReentrancyGuardUpgradeable instead of ReentrancyGuard

• Status: Rejected (Invalid Finding)
• Justification: This finding assumes the project is using OpenZeppelin Contracts v4.x. In OpenZeppelin Contracts v5.0, the upgradeable and non-upgradeable utility contracts were consolidated. The standard @openzeppelin/contracts/utils/ReentrancyGuard.sol contract now natively uses namespaced storage (ERC-7201) and is completely safe for upgradeable proxies. ReentrancyGuardUpgradeable no longer exists in v5.0.

[L-05] Pre-increment accountsLength in WERC7575ShareToken::consolidateTransfers()

• Status: Fixed
• Resolution: Applied the gas optimization by updating accountsLength++; to ++accountsLength; inside the consolidateTransfers() loop in WERC7575ShareToken.sol.

[L-06] ERC7575VaultUpgradeable::setOperator() emits the same exact event twice

• Status: Rejected (Design Requirement)
• Justification: The duplicate emission is an intentional architectural necessity. SukukFI implements a centralized delegation architecture where operators are approved at the ShareToken level, guaranteeing consistent authorization behavior across all associated vaults. However, the ERC7540 Specification explicitly states that setting an operator MUST emit the OperatorSet event from the vault contract being interacted with. Removing either event breaks either strict standard compliance for the vault or the centralized state tracking for the ShareToken.

[L-07] Deposit event deviates from ERC-7540 Specification

• Status: Fixed
• Resolution: Swapped the receiver and controller parameter order in the Deposit event emissions within ERC7575VaultUpgradeable.sol's deposit() and mint() functions to correctly match the standard specifications.

[L-08] Several events deviate from ERC-7887 Specification

• Status: Skipped (Already Mitigated)
• Resolution: Already addressed in the codebase prior to the review session.

[L-09] Input parameter, receiver, is not checked in several methods

• Status: Fixed
• Resolution: Added ERC20InvalidReceiver zero-address checks for receiver parameters in mint() and deposit() inside ERC7575VaultUpgradeable.sol.

[L-10] balanceOf() check is redundant

• Status: Fixed
• Resolution: Removed the inline logic validating ownerBalance < assets and ownerShares < shares in requestDeposit and requestRedeem inside ERC7575VaultUpgradeable.sol. The validation is securely handled natively via OpenZeppelin's underlying generic transfer operations (safeTransferFrom, vaultTransferFrom which leverages _transfer).

[L-11] scalingFactor check can be removed

• Status: Fixed
• Resolution: Removed the strict bounds restriction if (scalingFactor > type(uint64).max) revert ScalingFactorTooLarge(); from ERC7575VaultUpgradeable.sol.

[L-12] maxMint / maxDeposit limitations when inactive

• Status: Fixed
• Resolution: Updated maxDeposit and maxMint to dynamically check the isActive state boolean, returning 0 if the vault isActive boolean is false in ERC7575VaultUpgradeable.sol.