During the security assessment, it was found that the sUSDz contract was not properly calculating staked amounts after some yield was added using the addYield() function.

Specifically, the contract was not properly updating the pooledUSDz state variable. This was because the totalAssets() function, when there was some vesting unlocked, was returning the stored pooledUSDz plus the unlocked vesting amount. However, this was not stored in the globals state variable.

/**
 * @notice Total vested USDe in this contract.
 * @dev To prevent ERC4626 Inflation Attacks. We use pooledUSDz to calculate totalAssets instead of balanceOf().
 *
 * https://blog.openzeppelin.com/a-novel-defense-against-erc4626-inflation-attacks
 *
 */
function totalAssets() public view override returns (uint256) {
    // If all vested
    uint256 timeGap = block.timestamp.sub(lastDistributionTime);
    if (timeGap >= vestingPeriod) {
        if (vestingAmount != 0) {
            return pooledUSDz.add(vestingAmount);
        }
        return pooledUSDz;
    } else {
        uint256 unvestedAmount = ((vestingPeriod.sub(timeGap)).mul(vestingAmount)).div(vestingPeriod);
        uint256 vestedAmount = vestingAmount.sub(unvestedAmount);
        return pooledUSDz.add(vestedAmount);
    }
}

See the PoC section for a miscalculation example.

The following Foundry test was used in order to prove the aforementioned issue:

function testDepositAddYieldAndUnstake2Users() public {
    uint256 depositAmount = 1e20; // User to deposit 100 USDz
    uint256 depositAmount2 = 2e20; // User2 to deposit 200 USDz
    uint256 yieldAmount = 1e20; // 100 USDz yield
    uint256 initialUSDzBalance = usdzToken.balanceOf(user);
    uint256 initialUSDzBalance2 = usdzToken.balanceOf(user2);
    uint256 initialStakedUSDzBalance = vault.balanceOf(user);
    uint256 initialStakedUSDzBalance2 = vault.balanceOf(user2);

    // Logging the ratio
    console.log("1_000 assets to shares:", vault.convertToShares(1_000));

    testDeposit();
    assertEq(
        vault.balanceOf(user),
        initialStakedUSDzBalance + depositAmount,
        "User balance of sUSDz should be equal to initial deposit"
    );
    assertEq(
        vault.balanceOf(user2),
        initialStakedUSDzBalance2 + depositAmount2,
        "User2 balance of sUSDz should be equal to initial deposit"
    );

    // add yieldAmount to yield
    vm.prank(yieldManager);
    vault.addYield(yieldAmount);

    assertEq(
        vault.balanceOf(user),
        initialStakedUSDzBalance + depositAmount,
        "User balance of sUSDz should be unchanged right after yield is added"
    );
    assertEq(
        vault.balanceOf(user2),
        initialStakedUSDzBalance2 + depositAmount2,
        "User2 balance of sUSDz should be unchanged right after yield is added"
    );

    vm.warp(block.timestamp + vault.vestingPeriod());

    console.log("1_000 assets to shares changed:", vault.convertToShares(1_000));

    vm.startPrank(user);
    vault.deposit(depositAmount, user);
    assertEq(
        vault.balanceOf(user),
        initialStakedUSDzBalance + depositAmount + vault.convertToShares(depositAmount),
        "User balance of sUSDz should've increased by depositAmount"
    );

    // Cooldown all user assets and unstake
    vm.startPrank(user);
    vault.CDShares(vault.balanceOf(user));
    vm.warp(block.timestamp + vault.CDPeriod() + 1);

    console.log("1_000 assets to shares:", vault.convertToShares(1_000));
    assertEq(vault.balanceOf(user), 0, "User does not have sUSDz anymore ");
    assertEq(
        usdzToken.balanceOf(address(vault.flat())),
        depositAmount * 2 + yieldAmount / 3,
        "User's USDz are being help by USDzFlat"
    );
    assertEq(
        vault.balanceOf(user2),
        initialStakedUSDzBalance2 + depositAmount2,
        "User2 balance of sUSDz should be unchanged right after yield is added"
    );

    vm.startPrank(user);
    vault.unstake();
    assertEq(
        usdzToken.balanceOf(user),
        initialUSDzBalance + yieldAmount / 3,
        "User should receive deposit back plus yield"
    );
    assertEq(
        usdzToken.balanceOf(user2),
        initialUSDzBalance2 - depositAmount2,
        "User2 should have the initial minus the deposit, it hasn't unstake yet"
    );
    assertEq(vault.balanceOf(user), 0, "User does not have sUSDz anymore");
    assertEq(
        vault.balanceOf(user2),
        initialStakedUSDzBalance2 + depositAmount2,
        "User2 balance of sUSDz should be unchanged"
    );

    console.log("1_000 assets to shares:", vault.convertToShares(1_000));

    // Now user2
    // Cooldown all user2 assets and unstake
    // add yieldAmount to yield
    vm.stopPrank();
    vm.prank(yieldManager);
    vault.addYield(yieldAmount);
    vm.warp(block.timestamp + vault.vestingPeriod());

    assertEq(vault.balanceOf(user), 0, "User balance of sUSDz still has 0 sUSDz");
    assertEq(
        vault.balanceOf(user2),
        initialStakedUSDzBalance2 + depositAmount2,
        "User2 balance of sUSDz should be unchanged right after yield is added"
    );

    vm.startPrank(user2);
    vault.CDShares(vault.balanceOf(user2));
    vm.warp(block.timestamp + vault.CDPeriod() + 1);

    console.log("1_000 assets to shares:", vault.convertToShares(1_000));

    vault.unstake();
    assertEq(
        usdzToken.balanceOf(user2),
        initialUSDzBalance2 + 2 * yieldAmount / 3 + yieldAmount,
        "User2 should receive deposit back plus 2/3 of the 1st yield + whole 2nd yield"
    );

    console.log("1_000 assets to shares:", vault.convertToShares(1_000));

    console.log("sUSDz user: ", vault.balanceOf(user));
    console.log("sUSDz user2: ", vault.balanceOf(user2));
    console.log("sUSDz totalSupply: ", vault.totalSupply());
}

To run it, just execute the following forge command:

forge test --mt testDepositAddYieldAndUnstake2Users -vvvv

Observe that the test fails with an arithmetic error due to the second call to CDShares(). This is because, as indicated in the definition section, the pooledUSDz global state variable was not properly updated and the attempt to call the internal function _withdraw reverted.

    ├─ [49321] sUSDz::CDShares(200000000000000000000 [2e20])
    │   └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
    └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 8.74ms (1.13ms CPU time)

It is advisable to conduct a thorough review of the addYield() function to ensure that the introduction of additional yield does not disrupt the staking calculations. Specifically, make sure that any new yield amount is accurately reflected in the pooledUSDz and vestingAmount global state variables and that the unvested amount is properly calculated.

Remediation Plan

SOLVED: The Anzen team solved the issue by revising the addYield(), totalAssets() and getUnvestedAmount() functions to ensure that the new yield is accurately incorporated into the calculations.

The USDzPriceOracle and SPCTPriceOracle contracts use Chainlink as their price oracle. When requesting the price via the getPrice() function, the contracts query Chainlink for the underlying token price using the latestRoundData() function. This function returns uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt and uint80 answeredInRound. roundId denotes the identifier of the most recent update round, answer is the price of the asset, startedAt is the timestamp at which the round started and updatedAt is the timestamp at which the feed was updated.

The getPrice() function does not check if the feed was updated at the most recent round nor does it verify that startedAt (timestamp at which the round started) is not 0, and this can result in accepting stale data which may threaten the stability of the protocol in a volatile market.

function getPrice() external view returns (uint256) {
    (, int256 price,, uint256 updatedAt,) = priceFeed.latestRoundData();
    require(price > 0, "invalid price");
    require(block.timestamp - updatedAt <= heartbeat, "stale price");

    return uint256(price) * 10 ** (18 - uint256(priceFeed.decimals()));
}

Finally, be aware that, in the unlikely event of priceFeed.decimals() returning a value greater than 18, the whole function would break due to the last operation underflowing (18 - uint256(priceFeed.decimals())).

Consider improving checks for stale data. Specifically, make sure startedAt is not 0 (which would indicate that the round has not completed) and that answeredInRound is greater or equal to roundId.

Finally, consider adding a check for priceFeed.decimals() to make sure it is lower than or equal to 18.

function getPrice() external view returns (uint256) {
    (uint80 roundId, int256 price, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) = priceFeed.latestRoundData();
    require(price > 0, "invalid price");
    require(block.timestamp - updatedAt <= heartbeat, "stale price");
    require(startedAt > 0, "Round not complete");
    require(answeredInRound >= roundId, "stale price");

    uint256 priceFeedDecimals = uint256(priceFeed.decimals();
    require(priceFeedDecimals <= 18, "Incorrect decimals");

    return uint256(price) * 10 ** (18 - uint256(priceFeed.decimals()));
}

Remediation Plan

SOLVED: The Anzen team solved the issue by implementing the proposed improvement.

It was identified that the USDz contract inherited from OpenZeppelin's Ownable library. Ownership of the contracts that are inherited from the Ownable module can be lost, as the ownership is transferred in a single-step process. The address that the ownership is changed to should be verified to be active or willing to act as the owner. Ownable2Step is safer than Ownable for smart contracts because the owner cannot accidentally transfer smart contract ownership to a mistyped address. Rather than directly transferring to the new owner, the transfer only completes when the new owner accepts ownership.

The following Foundry test was used in order to prove the aforementioned issue:

function testUSDztransferOwnership() public {
  usdz.transferOwnership(address(1));
}

To run it, just execute the following forge command:

forge test --mt testUSDztransferOwnership -vvvv

Observe that the test passes and the new owner is now the invalid address(1).

[PASS] testUSDztransferOwnership() (gas: 10355)
Traces:
  [10355] HalbornUSDzTest::testUSDztransferOwnership
    ├─ [7240] USDz::transferOwnership(0x0000000000000000000000000000000000000001)
    │   ├─ emit OwnershipTransferred(previousOwner: HalbornUSDzTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], newOwner: 0x0000000000000000000000000000000000000001)
    │   └─ ← [Stop] 
    └─ ← [Stop] 

To mitigate the risks associated with single-step ownership transitions and enhance contract security, it is recommended to adopt a two-step ownership transition mechanism, such as OpenZeppelin's Ownable2Step. This approach introduces an additional step in the ownership transfer process, requiring the new owner to accept ownership before the transition is finalized. The process typically involves the current owner calling a function to nominate a new owner, and the nominee then calling another function to accept ownership.

Implementing Ownable2Step provides several benefits:

1. Reduces Risk of Accidental Loss of Ownership: By requiring explicit acceptance of ownership, the risk of accidentally transferring ownership to an incorrect or zero address is significantly reduced.

2. Enhanced Security: It adds another layer of security by ensuring that the new owner is prepared and willing to take over the responsibilities associated with contract ownership.

3. Flexibility in Ownership Transitions: Allows for a smoother transition of ownership, as the nominee has the opportunity to prepare for the acceptance of their new role.

By adopting Ownable2Step, contract administrators can ensure a more secure and controlled process for transferring ownership, safeguarding against the risks associated with accidental or unauthorized ownership changes.

It was noted that the affected contracts use the Ownable library though the LayerZeroLabs's OFT contracts. If directly changing from Ownable to Ownable2Step is not possible, consider overriding the affected functions to implement a two-step ownership transfer.

Remediation Plan

SOLVED: The Anzen team solved the issue by adding the logic to implement a two-step ownership transfer.

While it was identified that the used logic was extracted from the well-known OpenZeppelin library, it is also recommended to consider adding checks for cases for when the contract is paused or for addresses that are blacklisted. Some projects may not want contracts to be able to modify the owner address while paused or propose owners in the blacklisted list. Note that this is a consideration the project should deliberate on.

In the USDz contract, the checkCollateralRate() modifier is calling the _checkCollateralRate() internal function which, if the price returned by the oracle is less than 1, it pauses the contract and reverts with the UNDERCOLLATERAL_RATE,SMART_CONTRACT_IS_PAUSED_NOW error message. See:

modifier checkCollateralRate() {
  _checkCollateralRate();
  _;
}

/**
 * @notice Check collateral rate.
 */
function _checkCollateralRate() internal {
  if (oracle.getPrice() / 1e18 < collateralRate) {
      _pause();
      revert("UNDER_COLLATERAL_RATE,SMART_CONTRACT_IS_PAUSED_NOW");
  }
}

The modifier above is being used by the functions deposit(), depositBySPCT(), redeem() and redeemBackSPCT() to make sure the contract is paused immediately if the price falls below collateralRate. However, due to the nature of EVM transactions, the contract remains unpaused due to the revert() at the end.

function testUSDzcheckCollateralRateNotPausing() public {
    // Price decreases
    oracle.setPrice(1e12);

    // Cannot deposit due to under collateralization,
    // the USDz contract indicates that it has paused
    vm.expectRevert("UNDER_COLLATERAL_RATE,SMART_CONTRACT_IS_PAUSED_NOW");
    vm.prank(user);
    usdz.deposit(100e6);

    // According to the message above, USDz should be paused now
    // However, it isn't because the transaction reverted
    assertFalse(usdz.paused());
}

To mitigate this issue, it's recommended to revise the pausing mechanism to ensure effective contract pausing upon detecting undercollateralization. This may involve redesigning the modifier to properly handle pausing functionalities.

Remediation Plan

PARTIALLY SOLVED: The Anzen team solved the issue by removing the misleading call to the _pause() function. However, this finding has been marked as Partially Solved instead of Solved because the error message when undercollateralized is still giving the wrong information that the contract is now paused: "UNDER_COLLATERAL_RATE,SMART_CONTRACT_IS_PAUSED_NOW".

Even though the contracts in scope are importing the OpenZeppelin's SafeERC20's library, they are not always making use of its secure functions (safeTransfer(), safeTransferFrom() , safeIncreaseAllowance() and safeDecreaseAllowance() ) and use the insecure versions instead (transfer(), transferFrom() and approve()). Notice that, in some cases the insecure function is not even checking the returned value, see the example below using the insecure transferFrom() and not checking its returned value:

function deposit(uint256 _amount) external whenNotPaused {
  require(_amount > 0, "DEPOSIT_AMOUNT_IS_ZERO");

  usdc.transferFrom(msg.sender, address(this), _amount);

Use OpenZeppelin's SafeERC20's safeTransfer(), safeTransferFrom() , safeIncreaseAllowance() and safeDecreaseAllowance() instead transfer(), transferFrom() and approve().

Remediation Plan

PARTIALLY SOLVED: The Anzen team fixed some occurrences of unsafe function calls not checking the returned values. However, there are still some instances using unsafe calls like the deposit() function of SPCTPool which is using transferFrom().

It was identified that the USDz contract inherited from OpenZeppelin's Ownable library. In the Ownable contracts, the renounceOwnership() function is used to renounce the Owner permission. Renouncing ownership before transferring would result in the contract having no owner, eliminating the ability to call privileged functions.

/**
 * @dev Leaves the contract without owner. It will not be possible to call
 * `onlyOwner` functions. Can only be called by the current owner.
 *
 * NOTE: Renouncing ownership will leave the contract without an owner,
 * thereby disabling any functionality that is only available to the owner.
 */
function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

The following Foundry test was used in order to prove the aforementioned issue:

function testUSDzrenounceOwnership() public {
  usdz.renounceOwnership();
}

To run it, just execute the following forge command:

forge test --mt testUSDzrenounceOwnership -vvvv

Observe that the test passes and the ownership has been renounced (address(0)).

[PASS] testUSDzrenounceOwnership() (gas: 5291)
Traces:
  [8073] HalbornUSDzTest::testUSDzrenounceOwnership()
    ├─ [7052] USDz::renounceOwnership()
    │   ├─ emit OwnershipTransferred(previousOwner: HalbornUSDzTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], newOwner: 0x0000000000000000000000000000000000000000)
    │   └─ ← [Stop] 
    └─ ← [Stop] 

It is recommended that the Owner cannot call renounceOwnership() without first transferring ownership to another address. In addition, if a multi-signature wallet is used, the call to the renounceOwnership() function should be confirmed for two or more users.

Remediation Plan

ACKNOWLEDGED: The Anzen team acknowledged the issue.

During the security assessment, it was found that critical parameters could be set to undesired values such as address(0) or unrealistic numbers. For instance:

1. Constructor Zero Address Checks: Some constructors of the project lack validation to ensure that zero addresses are not accepted for essential parameters. This omission could lead to unexpected behavior due to non-initialized values.

2. Constructor Unbounded Values: Apart from addresses, some of the constructors contained integer values that were not checked against valid ranges. For example, on sUSDz, any cooldown period (_CDPeriod) would be accepted even though not any value may be desirable.

3. Setter Functions Validation: Furthermore, even some setters did not have address 0 or boundary checks either. Notice a newPeriod or 0 or type(uint256).max could be set using the setNewVestingPeriod() function of sUSDz:

function setNewVestingPeriod(uint256 newPeriod) external onlyRole(POOL_MANAGER_ROLE) {
  vestingPeriod = newPeriod;
  emit VestingPeriodChanged(newPeriod, block.timestamp);
}

To address these issues, consider implementing input validation checks to ensure that zero addresses or unwanted values are not accepted for essential parameters.

Remediation Plan

PARTIALLY SOLVED: The Anzen team partially solved the issue by implemented a valid check in the SUSDz constructor. However, there are still other functions missing range or address validations.

Finally, it was noted that one of the added fixes is not provideiding a useful check. This is because it is not properly checking any range value. See the new require statement in the setNewVestingPeriod(uint256 newPeriod) function:

require(newPeriod > 0 && newPeriod < type(uint256).max, "SHOULD_BE_LESS_THAN_UINT256_MAX_AND_GREATER_THAN_ZERO");

Notice that verifying if the newPeriod value is between the values 0 and type(uint256).max is like not doing any verification at all, as all uint256 fall into that range.

The USDz and SPCTPool contracts contained several require() statements like the following:

require(newMintFeeRate <= maxMintFeeRate, "SHOULD_BE_LESS_THAN_1P");

It was observed that, even though the error message says SHOULD_BE_LESS_THAN_1P, the value could actually be equal to 1% (maxMintFeeRate).

In order to improve consistency, consider adapting the require statements to:

require(newMintFeeRate < maxMintFeeRate, "SHOULD_BE_LESS_THAN_1P");

or

require(newMintFeeRate <= maxMintFeeRate, "SHOULD_BE_LESS_THAN_OR_EQUAL_TO_1P");

Remediation Plan

SOLVED: The Anzen team solved the issue by adjusting the error message to the actual behaviour of the condition.

Essential contract settings like mint fee rates, redeem fee rates and treasury addresses are not configured in the constructors of the affected contracts. This may break certain functionalities if those values are partially set afterwards.

For instance, in the USDz contract, if mintFeeRate is set without a corresponding treasury address, functionalities like deposit() and depositBySPCT() may stop working.

Though the risk associated with this finding is low, it is advisable to enhance contract robustness by including explicit initializations for mint fees, redeem fees, and treasury addresses (if applicable) in the constructor. This preventive measure ensures a complete configuration during contract deployment, minimizing the possibility of operational disruptions due to partially set values.

Additionally, addressing this issue during deployment could simplify contract management and maintenance, contributing to a more streamlined and reliable operational framework.

Remediation Plan

ACKNOWLEDGED: The Anzen team acknowledged the issue.

At least all public and external functions that are not view or pure should have NatSpec comments. During the security review, it was observed that the multiple publicly-accessible functions did not have any NatSpec comments.

Moreover, the test suite's coverage is insufficient, representing a less-than-ideal practice and failing to ensure thorough validation of function behaviors and overall contract functionality.

Consider adding NatSpec documentation to all functions, specially those that are publicly-accessible.

Additionally, expanding the test suite's coverage is essential to effectively validate function functionalities and ensure thorough contract testing. Enhancing the test suite represents a best practice, instilling confidence in contract behavior, mitigating the risk of undetected bugs, and streamlining the development process. By addressing these areas, the project can foster transparency, reliability, and robustness, thereby enhancing its overall quality and user experience.

Remediation Plan

ACKNOWLEDGED: The Anzen team acknowledged the issue.

In Solidity smart contract development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

It is recommended to replace hard-coded revert strings in require statements for custom errors, which can be done following the logic below.

1. Standard require statement (to be replaced):

require(condition, "Condition not met");

2. Declare the error definition to state

error ConditionNotMet();

3. As currently is not possible to use custom errors in combination with require statements, the standard syntax is:

if (!condition) revert ConditionNotMet();

More information about this topic in Official Solidity Documentation.

Remediation Plan

ACKNOWLEDGED: The Anzen team acknowledged the issue.

The SPCTPriceOracle and USDzPriceOracle contracts contained the hardcoded address 0xAed0c38402a5d19df6E4c03F4E2DceD6e29c1ee9 as the SPCT/USD and USDz/USD oracle addresses respectively. However, this was found to be the DAI/USD Chainlink oracle on Ethereum mainnet.

This finding was only rated as informational risk because it is believed that the codebase is still not in production and will be updated before deploying. However, be aware that going live with the current configuration could lead to disastrous results.

Make sure the oracle addresses in SPCTPriceOracle and USDzPriceOracle contracts are correct to avoid potential catastrophic outcomes.

Remediation Plan

ACKNOWLEDGED: The Anzen team acknowledged the issue.

It has been observed that important functionalities are missing emitting events.

Events are a method of informing the transaction initiator about the actions taken by the called function. It logs its emitted parameters in a specific log history, which can be accessed outside of the contract using some filter parameters. Events help non-contract tools to track changes, and events prevent users from being surprised by changes.

All functions updating important parameters should emit events.

Remediation Plan

ACKNOWLEDGED: The Anzen team acknowledged the issue.

Indexed event fields make the data more quickly accessible to off-chain tools that parse events, and add them to a special data structure known as "topics" instead of the data part of the log. A topic can only hold a single word (32 bytes) so if you use a reference type for an indexed argument, the Keccak-256 hash of the value is stored as a topic instead.

Each event can use up to three indexed fields. If there are fewer than three fields, all the fields can be indexed. It is important to note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed fields per event (three indexed fields).

This is specially recommended when gas usage is not particularly of concern for the emission of the events in question, and the benefits of querying those fields in an easier and straight-forward manner surpasses the downsides of gas usage increase.

Notice the following events not using any indexed field:

• SPCTPool.sol:

event Execute(uint256 amount, uint256 timestamp);
event Repay(uint256 amount, uint256 timestamp);

event mintFeeRateChanged(uint256 newFeeRate, uint256 timestamp);
event redeemFeeRateChanged(uint256 newFeeRate, uint256 timestamp);
event treasuryChanged(address newTreasury, uint256 timestamp);

• sUSDz.sol:

event YieldReceived(uint256 amount, uint256 timestamp);
event CDPeriodChanged(uint256 newCDPeriod, uint256 timestamp);
event VestingPeriodChanged(uint256 newCDPeriod, uint256 timestamp);

• USDz.sol:

event mintFeeRateChanged(uint256 newFeeRate, uint256 timestamp);
event redeemFeeRateChanged(uint256 newFeeRate, uint256 timestamp);
event treasuryChanged(address newTreasury, uint256 timestamp);
event oracleChanged(address newOracle, uint256 timestamp);

• vault.sol:

event IncreaseCap(uint256 amount, uint256 timestamp);
event Execute(uint256 amount, uint256 timestamp);

Finally, it was noted that, as shown above, most events used timestamp as an argument. Notice that block.timestamp and block.number are added to event information by default, so adding them manually wastes gas.

It is recommended to add the indexed keyword when declaring events, considering the following example:

event treasuryChanged(address indexed newTreasury, uint256 timestamp);

Finally, consider removing the timestamp argument from all the events. So, an even better approach would be:

event TreasuryChanged(address indexed oldTreasury, address indexed newTreasury);

Remediation Plan

PARTIALLY SOLVED: The Anzen team solved the issue by adding indexed attributes to most of the events and removing the timestamp redundant value. Notice that this finding is not marked as solved because some events are still missing the mentioned attribute and the src/interfaces/IsUSDz.sol file is still referencing events with timestamps.

Within the Anzen protocol v2, a suite of interfaces is provided in the /src/interfaces/ directory to define contract specifications. However, it was discovered that none of the contract implementations are inheriting from those interfaces. This oversight poses a risk as it lacks a crucial mechanism for ensuring consistency between interface declarations and their implementations. Inheritance of interfaces guarantees the alignment of function signatures, thereby preventing inadvertent errors resulting from incorrect implementations.

During testing, efforts were made to establish inheritance between the contract implementations and their corresponding interfaces. Unfortunately, these attempts were unsuccessful, leading to errors such as Missing implementation and Function overload clash.

It is recommended to fix this issue by enforcing inheritance between contract implementations and their respective interfaces. This step is crucial for maintaining code integrity and preventing discrepancies between interface specifications and their implementations. By adhering to this practice, the risk of introducing errors due to mismatched function signatures can be mitigated effectively.

Remediation Plan

ACKNOWLEDGED: The Anzen team acknowledged the issue.

In the USDz.sol, childUSDz.sol and Canto_childUSDz.sol files, some of the imported contracts were found to be unused. More specifically:

On USDz.sol:

import "forge-std/Test.sol";

On childUSDz.sol and Canto_childUSDz.sol:

import {
    SendParam,
    OFTReceipt,
    MessagingReceipt,
    MessagingFee
} from "@layerzerolabs/lz-evm-oapp-v2/contracts/oft/interfaces/IOFT.sol";

Unused imports should be cleaned up from the code if they have no purpose. Clearing these imports can reduce gas usage during the deployment of contracts, and also increases the readability of the code.

Remediation Plan

SOLVED: The Anzen team solved the issue by removing the unused imports.

During the security review, it was noted that the code of the file src/utils/SafeMath.sol was extracted from an older version of the popular OpenZeppelin library.

As a general best-security practice, it is highly advisable to consistently review all third-party code being utilized and meticulously assess the accompanying security audits.

For this particular case, we suggest removing the use of SafeMath altogether, as it does not need anymore since Solidity version 0.8.0. Notice, the version of OpenZeppelin in use by the project (v5) does not have such library anymore.

Remediation Plan

ACKNOWLEDGED: The Anzen team acknowledged the issue.

The files in scope currently use floating pragma version ^0.8.21, which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.21, and less than 0.9.0.

It is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Moreover, in the foundry.toml configuration file, the project is declaring version = "0.8.24" instead of solc_version = "0.8.24".

Lock the pragma version to the same version used during development and testing.

For the foundry.toml configuration file, consider changing the declaration version = "0.8.24" to solc_version = "0.8.24".

Remediation Plan

PARTIALLY SOLVED: The Anzen team fixed the (out-of-scope) foundry.toml configuration file. However, the source code files in scope are still using a floating pragma (pragma solidity ^0.8.21).

State variables that are not updated following deployment should be declared constant to save gas.

uint256 public collateralRate = 1;

Consider adding the constant attribute to state variables that never change.

Remediation Plan

SOLVED: The Anzen team solved the issue by adding the constant attribute to the collateralRate state variable.

It is best practice to set the visibility of state variables and constants explicitly. In the SPCTPriceOracle.sol and USDzPriceOracle.sol contracts, the heartbeat and SPCT_USD_ORACLE_ADDRESS constants did not have the visibility set:

uint256 constant heartbeat = 2 hours;
address constant SPCT_USD_ORACLE_ADDRESS = 0xAed0c38402a5d19df6E4c03F4E2DceD6e29c1ee9;

Consider explicitly setting the visibility of all state variables and constants.

Remediation Plan

SOLVED: The Anzen team solved the issue by explicitly setting the visibility to the mentioned constants.

Several comments in the sUSDz.sol file appear to be wrong or outdated:

1. * @notice Add Yield(USDe) to this contract.

2. * @notice Total vested USDe in this contract.

3. * @notice Used to claim USDe after CD has finished.

These comments reference to a token named USDe, which is not utilized or mentioned anywhere else in the project's codebase. It seems that these comments were extracted from an older version of the codebase and do not accurately reflect the current implementation.

To address these issues:

1. Review and Update Comments: Conduct a thorough review of all comments in the sUSDz.sol file. Identify any comments that reference unused or outdated features, such as USDe.

2. Remove or Update Outdated Comments: For comments referencing features that are no longer used in the project, consider removing them entirely to avoid confusion. Alternatively, if the comments are still relevant but inaccurately describe the current implementation, update them to reflect the correct information.

3. Maintain Documentation Consistency: Ensure that all comments accurately describe the functionality of the codebase and align with the current implementation. Regularly review and update comments as necessary to maintain consistency and clarity in the codebase documentation.

By implementing these remediation steps, you can improve the accuracy and clarity of the codebase comments, making it easier for developers to understand and maintain the project.

Remediation Plan

SOLVED: The Anzen team solved the issue by removing the outdated comments.

A few typographical errors were found in error messages and comments:

• On sUSDz.sol:

require(_assets <= maxWithdraw(msg.sender), "WITHDRAW_AMOUNT_EXECEED");

EXECEED should be EXCEEDED.

require(_shares <= maxRedeem(msg.sender), "WITHDRAW_AMOUNT_EXECEED");

EXECEED should be EXCEEDED

• On vault.sol:

// If is USDC, check total excuted amount first.

excuted should be executed

• On SPCTPool.sol:

* @dev SPCT only avaliable for KYC Users.

avaliable should be available

To maintain clarity and trustworthiness, it is essential to rectify any typographical errors present within the contracts. Correcting such errors minimizes the likelihood of confusion and reinforces confidence in the accuracy and integrity of the documentation.

Remediation Plan

SOLVED: The Anzen team solved the issue by correcting the typographical errors found.

During the code review, several instances of non-compliant naming conventions were identified in the Solidity codebase. Specifically:

• Some contract names were not in CapWords:

  • childUSDz and sUSDz

• Some constants were not in uppercase:

  • uint256 constant heartbeat = 2 hours;

  • uint256 public constant maxMintFeeRate = FEE_COEFFICIENT / 100;

  • uint256 public constant maxRedeemFeeRate = FEE_COEFFICIENT / 100;

• Some events are not in CapWords:

  • event mintFeeRateChanged(uint256 newFeeRate, uint256 timestamp);

  • event redeemFeeRateChanged(uint256 newFeeRate, uint256 timestamp);

  • event treasuryChanged(address newTreasury, uint256 timestamp);

  • event mintFeeRateChanged(uint256 newFeeRate, uint256 timestamp);

  • event redeemFeeRateChanged(uint256 newFeeRate, uint256 timestamp);

  • event treasuryChanged(address newTreasury, uint256 timestamp);

  • event oracleChanged(address newOracle, uint256 timestamp);

• The getUnvestedAmount() function of the sUSDz contract has a name implyting that it only "gets" a value (of view type), but it is actually updating some variables.

To ensure consistency and improve code readability, it is recommended to adhere strictly to the naming conventions specified by the Solidity language. Contract names should be in CapWords, constants should be entirely capitalized, and event names should follow the CapWords convention. Adopting these standard naming practices will enhance the clarity and maintainability of the codebase, making it easier for developers to understand and maintain the code over time.

Additionally, enforcing these conventions through code reviews and automated linting tools can help maintain consistency across the project and prevent future deviations from the standard naming conventions.

Remediation Plan

PARTIALLY SOLVED: The Anzen team solved the issue by following naming convention more accurately. It is important to note that there are still some room for improvement like the collateralRate, maxMintFeeRate, maxRedeemFeeRate and heartbeat constants (should be in UPPER_CASE), the vault.sol and childUSDz.sol file names (could be renamed to Vault.sol and ChildUSDz.sol respectively) and childUSDz contract inside the file Canto_childUSDz.sol (could be renamed to CantoChildUSDz or ChildUSDz).

In the contract SPCTPool, a tautology expression was detected because it is checking an unsigned integer (uint256) is greater than 0. Such expressions are of no use since they always evaluate to true regardless of the parameters they receive.

function repay(uint256 _amount) external onlyRole(POOL_MANAGER_ROLE) {
  require(_amount > 0, "REPAY_AMOUNT_IS_ZERO");
  require(executedUSD.sub(_amount) >= 0, "REPAY_AMOUNT_EXCEED_EXECUTED_SHARES");
  ...

Furthermore, two boolean equalities were found in the USDz contract. This means comparing to a constant (true or false), which is a bit more gas expensive than directly checking the boolean value.

require(mode == false, "PLEASE_MIGRATE_TO_NEW_VERSION");

For the detected tautology, if the intention is to make sure amount is not greater than executedUSD, consider the following change:

if(amount > executedUSD) {
  revert("REPAYAMOUNT_EXCEED_EXECUTED_SHARES");
}

For the boolean equalities, consider using if(!mode) instead of require(mode == false).

Remediation Plan

PARTIALLY SOLVED: The Anzen team partially solved the issue by fixing most of the require statements. However, for the boolean equality, only the one present in the deposit() was fixed, but there is one remaining in the depositBySPCT() function. Finally, the require to check if repay() was called with an _amount of 0 was removed, but it is recommended to add it back to prevent the function from emitting events with 0 value.

During the security assessment, it was observed that both the internal function _checkCollateralRate() within the USDz contract and the _updateVestingAmount() internal function within the sUSDz contract were only invoked once.

While modular design often enhances code readability and maintainability, in instances where internal functions are utilized singularly, it can introduce unnecessary complexity and hinder code comprehension. This redundancy not only obscures the logic flow but also increases the cognitive load for developers maintaining or auditing the codebase.

Instead of separating the logic into a separate function, consider inlining the logic into the calling function or modifier. This can reduce the number of function calls and improve readability.

Remediation Plan

ACKNOWLEDGED: The Anzen team acknowledged the issue.

The following events are defined but never emitted.

event Mint(address indexed user, uint256 amount, uint256 timestamp);
event Burn(address indexed user, uint256 amount, uint256 timestamp);
event Repay(uint256 amount, uint256 timestamp);

Unused events can be removed to make the code cleaner.

Remediation Plan

SOLVED: The Anzen team solved the issue by either start using or removing the unused events.