Archimedes Finance - Archimedes

Once any user tries to unwind a position, one of the multiple steps performed is to redeem every OUSD available in the vault under that position and:

1. Check the LVUSD amount that was initially borrowed and has to be repaid in order to close the position.
2. Calculate the needed amount of OUSD that will have to be exchanged for the amount of LvOUSD obtained in the previous step (plus an extra 1% for slippage protection).
3. Check that the needed amount of OUSD is not greater than the OUSD available. Otherwise, the user will not be able to close the position since he cannot repay the LvOUSD initially borrowed.

Then, this minimum required amount of OUSD calculated is swapped for LvOUSD, and the output is then compared again with the initially borrowed LvOUSD.

If the amount of swapped LvOUSD is enough to repay the initially borrowed LvOUSD, it will be transferred back to the Coordinator, and the remaining OUSD amount will be transferred to the user as collateral reimbursement (and profits, if any).

However, if the calculated amount of needed OUSD does not finally get as many LvUSD as calculated (due to slippage, imbalanced pool, etc.), the transaction will be reverted with Not enough LvUSD in pool error, remaining unwindable until the OUSD amount calculated for swapping actually gets enough LvUSD back, although more than enough OUSD for closing the position is held.

This behavior goes against what is described in Archimedes Documentation, which states that any user holding enough OUSD to pay for the debt should be able to close the position:

It also has been detected that _neededOUSDWithSlippage is not being accurately calculated since it assumes that the estimated amount calculated using get_dy functions will remain constant in both directions of the swap (LvUSD -> 3CRV -> OUSD and OUSD -> 3CRV -> LvUSD), and only a 1% slippage protection has been hardcoded (which would have to absorb swap miscalculations, actual slippage, etc.), which might be not enough and introduces an artificial position unwind limit of ~2.3%, meaning that any position borrowing a LvUSD amount greater than 2.3% of the total pool balance (LvUSD + 3CRV balances) will become unwindable.

Code Location

Exchanger.sol

       function _swapOUSDforLvUSD(uint256 amountOUSD, uint256 minRequiredLvUSD) internal returns (uint256 lvUSDReturned, uint256 remainingOUSD) {
        // Estimate "neededOUSD" using get_dy()
        uint256 _needed3CRV = _poolLvUSD3CRV.get_dy(_indexLvUSD, _index3CRV, minRequiredLvUSD);
        uint256 _neededOUSD = _poolOUSD3CRV.get_dy(_index3CRV, _indexOUSD, _needed3CRV);
        uint256 _neededOUSDWithSlippage = (_neededOUSD * 101) / 100;

        require(amountOUSD >= _neededOUSDWithSlippage, "Not enough OUSD for exchange");

        // We lose some $ from fees and slippage
        // multiply _neededOUSD * 103%
        uint256 _returned3CRV = _xOUSDfor3CRV(_neededOUSDWithSlippage);

        uint256 _returnedLvUSD = _x3CRVforLvUSD(_returned3CRV);
        require(_returnedLvUSD >= minRequiredLvUSD, "Not enough LvUSD in pool");

        // calculate remaining OUSD
        remainingOUSD = amountOUSD - _neededOUSDWithSlippage;
        _ousd.safeTransfer(_addressCoordinator, remainingOUSD);

        // send all swapped lvUSD to coordinator
        _lvusd.safeTransfer(_addressCoordinator, _returnedLvUSD);

        return (_returnedLvUSD, remainingOUSD);
    }

A new position has been opened by user1, depositing 10 OUSD for 10 cycles. When trying to unwind the position, the transaction will get reverted with a Not enough LvUSD in pool error. After that, calltrace will be shown in order to inspect the relevant values involved (only relevant values will be displayed):

This is the call performed to Exchanger.swapOUSDforLvUSD. The total amount of OUSD held in the position is 65.44831518184307 OUSD, and the minimum amount of LvUSD to repay is 58.618940391 LvUSD:

And the amount of LvUSD received by the calculated amount of OUSD is 58.4852340246938 LvUSD, which is lower than the minimum amount needed:

The user has indeed enough OUSD to close the position, but not enough are taken.

To ilustrate the _neededOUSDWithSlippage miscalculation, a few swap estimations have been performed using the contract's implemented logic. The initial condition is a balanced LvUSD/3CRV pool with 100_000 LvUSD and 100_000 3CRV (assuming no slippage):

An alternative method to estimate the OUSD amount to be swapped can be used, preventing transactions from reverting and actually allowing users to unwind their positions:

When any user calls createLeveragedPosition() from LeverageEngine contract, depositCollateralUnderNFT() is called, and this function also calls archimedesDeposit() from the OUSD Vault, in order to actually deposit the OUSD provided by the user in the vault and issue the corresponding vault shares.

One of the instructions executed when calling archimedesDeposit() is to call _takeRebaseFees(), which compares the current OUSD balance of the pool (by calling OUSD.balanceOf()) with the last known amount of OUSD held in the vault (stored in _assetsHandledByArchimedes) to look for any possible OUSD rebase, which would have increased vault's OUSD balance.

However, it has been detected that due to the behavior of OUSD when querying for balances described in the previous finding, _assetsHandledByArchimedes could be greater than totalAssets(), causing an integer underflow every time _takeRebaseFees() gets called, and effectively blocking the contracts (since no position could be opened or unwound in this state) until OUSD gets rebased (making totalAssets() greater or equal than _assetsHandledByArchimedes).

Code Location

VaultOUSD.sol

    function _takeRebaseFees() internal {
        uint256 unhandledRebasePayment = totalAssets() - _assetsHandledByArchimedes;
        /// only run fee collection if there are some rebased funds not handled
        if (unhandledRebasePayment > 0) {
            uint256 feeToCollect = (unhandledRebasePayment * _paramStore.getRebaseFeeRate()) / 1 ether;
            uint256 handledRebaseValueToKeepInVault = unhandledRebasePayment - feeToCollect;

            _assetsHandledByArchimedes += handledRebaseValueToKeepInVault;

            _ousd.transfer(_paramStore.getTreasuryAddress(), feeToCollect);
        }
    }

In order to improve variable visibility, _assetsHandledByArchimedes has been declared as public instead of internal.

For this proof of concept, a user will try to open two consecutive positions, and values used in _takeRebaseFees will be analyzed:

Opening a position requires the following user input:

• Amount of OUSD that will be used as collateral
• Number of leverage cycles
• The required amount of ARCH to burn for the position

When calling createLeveragedPosition() from LeverageEngine.sol contract, the first instruction calculates the corresponding LvUSD to be unlocked with the provided collateral and leverage cycles. The second one checks the maximum amount of LvUSD that could be claimed with the ARCH amount supplied, reverting if the result of the second instruction is lower than the first one.

This effectively checks if the user provided at least the minimum amount of ARCH required for opening the position, but it does not check if the user provided more ARCH than is actually needed.

The next instruction just sends the ARCH amount provided by the user to the treasury account, effectively taking all of the ARCH provided.

Code Location

LeverageEngine.sol

function createLeveragedPosition(
        uint256 ousdPrinciple,
        uint256 cycles,
        uint256 archAmount
    ) external nonReentrant returns (uint256) {
        uint256 lvUSDAmount = _parameterStore.getAllowedLeverageForPosition(ousdPrinciple, cycles);
        uint256 lvUSDAmountAllocatedFromArch = _parameterStore.calculateLeverageAllowedForArch(archAmount);
        /// Revert if not enough Arch token for needed leverage. Continue if too much arch is given
        require(lvUSDAmountAllocatedFromArch >= lvUSDAmount, "Not enough Arch provided");

        uint256 availableLev = _coordinator.getAvailableLeverage();
        require(availableLev >= lvUSDAmount, "Not enough available lvUSD");

        _burnArchTokenForPosition(msg.sender, archAmount);
        uint256 positionTokenId = _positionToken.safeMint(msg.sender);
        _ousd.safeTransferFrom(msg.sender, _addressCoordinator, ousdPrinciple);
        _coordinator.depositCollateralUnderNFT(positionTokenId, ousdPrinciple);
        _coordinator.getLeveragedOUSD(positionTokenId, lvUSDAmount);

        emit PositionCreated(msg.sender, positionTokenId, ousdPrinciple, lvUSDAmount, archAmount);
        return positionTokenId;
    }

A new position has been opened by user1, depositing 10 OUSD for 10 cycles. The amount of ARCH needed for opening the position is 58, but the user supplies 1000, just to be sure.

The calculateArchNeededForLeverage() function is defined within the ParameterStore.sol contract, and its intended use is to calculate how much ARCH needs to be sent along with the initial amount of OUSD to open the position.

However, it has been detected that the calculation is performed incorrectly due to the order of the operations performed.

This is how the ARCH amount is calculated:

\begin{center} \begin{math} (leverageAmount / _archToLevRatio) * 1 ether \end{math} \end{center}

Since the division is performed before the multiplication, the result of the division will be rounded down to the closest integer, having that if the real amount of ARCH needed is 58.751, the result of the calculation will be 58.

If this function is used in the frontend or is manually called by any user, will either prevent users from opening any position using the right amount of ARCH needed (or having to calculate it using less intuitive methods) or force them to send more ARCH than needed to open the position.

In addition to this, and if calculateArchNeededForLeverage() is used in createLeveragedPosition() logic in the future, this behavior may allow users opening positions without having to pay any ARCH if the result of the division is lower than 1.

This finding would usually be classified as Medium, but, since extra ARCH tokens are not returned as described in HAL-04, the impact of this finding has been raised.

Code Location

ParameterStore.sol

    function calculateArchNeededForLeverage(uint256 leverageAmount) public view returns (uint256) {
        return (leverageAmount / _archToLevRatio) * 1 ether;
    }

A new position will be opened by user1, depositing 10 OUSD for 10 cycles. The amount of ARCH needed for opening the position, according to the output of calculateArchNeededForLeverage is 58, which is the amount the user will be supplied. Since the calculation has been rounded down, it will not create the position, since not enough ARCH has been provided. If supplying 100 ARCH instead, the position will be created successfully.

Every main contract has a setDependencies() function, which is used to set the key contracts needed for the proper functioning of the entire ecosystem, such as tokens, pools, or other contract addresses, or modify them after the deployment if needed for operational purposes.

In addition to that, Coordinator.sol and PoolManager.sol contracts include safeApprove() and approve() calls, respectively.

Due to these calls and the implementation of safeApprove() and 3CRV token's approve() function, which prevents new safeApprove() or approve() calls if the current allowance is greater than zero, setDependencies() could remain blocked, preventing contract administrators to modify these values if needed.

Code Location

PoolManager.sol

    function setDependencies(
        address addressParameterStore,
        address addressCoordinator,
        address addressLvUSD,
        address address3CRV,
        address addressPoolLvUSD3CRV
    ) external nonReentrant onlyAdmin {
        // Set variables
        _addressParameterStore = addressParameterStore;
        _addressCoordinator = addressCoordinator;
        _addressPoolLvUSD3CRV = addressPoolLvUSD3CRV;

        // Load contracts
        _paramStore = ParameterStore(addressParameterStore);
        _lvusd = IERC20Upgradeable(addressLvUSD);
        _crv3 = IERC20Upgradeable(address3CRV);
        _poolLvUSD3CRV = ICurveFiCurve(addressPoolLvUSD3CRV);

        _lvusd.approve(_addressPoolLvUSD3CRV, type(uint256).max);
        _crv3.approve(_addressPoolLvUSD3CRV, type(uint256).max);
    }

PoolManager.sol calls 3CRV.approve() each time setDependencies() is called. Since 3CRV's implementation of approve() does not allow new approvals when there is already an existing one, every setDependencies() call after the first one will be reverted.

Calling setDependencies() more than once on PoolManager.sol and Coordinator.sol contracts will easily disclose the problem:

It has been detected that some parameters used in the different contracts are set and used in a way that lets the minimum settable value at 1%.

Code Location

ParameterStore.sol

        _maxNumberOfCycles = 10;
        _originationFeeRate = 5 ether / 100;
        _globalCollateralRate = 90;
        _rebaseFeeRate = 10 ether / 100; // meaning 10%
        _treasuryAddress;
        _curveGuardPercentage = 90;
        _slippage = 2; // 2%;
        _archToLevRatio = 1 ether; // meaning 1 arch is equal 1 lvUSD
        _curveMaxExchangeGuard = 50; // meaning we allow exchange with get 50% more then we expected
        _treasuryAddress = address(0);

For example, _slippage is set to 2, implying that a maximum slippage of 2% is allowed. However, if this value needs to be modified in the future or if user-controlled slippage is implemented, it would only accept integer values, such as 0%, 1%, 2%, etc.

When a require condition is not satisfied, an error message is usually included in the response, giving information about the reason for the revert.

However, it has been detected that some of the error messages displayed when certain conditions are not met provide an incorrect explanation for the revert. These inaccurate messages could hinder the identification of the cause of the errors to contract administrators and could lead to contract malfunctions if managing decisions are taken after these error messages.

Code Location

Four different functions have been implemented for exchanging LvUSD, OUSD, and 3CRV pools between Curve pools. However, the error message LvUSD pool too imbalanced. is displayed on _xLvUSDfor3CRV, _x3CRVforLvUSD, and _x3CRVforOUSD, but OUSD pool too imbalanced. should be displayed instead in the last function.

Exchanger.sol

function _xOUSDfor3CRV(uint256 amountOUSD) internal returns (uint256 amount3CRV) {
        /**
         * @param _expected3CRV uses get_dy() to estimate amount the exchange will give us
         * @param _minimum3CRV minimum accounting for slippage. (_expected3CRV * slippage)
         * @param _returned3CRV amount we actually get from the pool
         * @param _guard3CRV sanity check to protect user
         */
        uint256 _expected3CRV;
        uint256 _minimum3CRV;
        uint256 _returned3CRV;
        uint256 _guard3CRV = (amountOUSD * _paramStore.getCurveGuardPercentage()) / 100;

        // Verify Exchanger has enough OUSD to use
        require(amountOUSD <= _ousd.balanceOf(address(this)), "Insufficient OUSD in Exchanger.");

        // Estimate expected amount of 3CRV
        // get_dy(indexCoinSend, indexCoinRec, amount)
        _expected3CRV = _poolOUSD3CRV.get_dy(0, 1, amountOUSD);

        // Set minimum required accounting for slippage
        _minimum3CRV = (_expected3CRV * (100 - _paramStore.getSlippage())) / 100;

        // Make sure pool isn't too bent
        // TODO allow user to override this protection
        // TODO auto balance if pool is bent
        require(_minimum3CRV >= _guard3CRV, "OUSD pool too imbalanced.");

        // Increase allowance
        _ousd.safeIncreaseAllowance(address(_poolOUSD3CRV), amountOUSD);

        // Exchange OUSD for 3CRV:
        _returned3CRV = _poolOUSD3CRV.exchange(0, 1, amountOUSD, _minimum3CRV);

        // Set approval to zero for safety
        _ousd.safeApprove(address(_poolOUSD3CRV), 0);

        return _returned3CRV;
    }

The Owner of the contract is usually the account that deploys the contract. As a result, the Owner can perform some privileged functions (such as adding minters or set pools). In every smart contract in scope, ownership renounce functions can be used to renounce the Owner or ADMIN permission/role. Renouncing ownership before transferring would result in the contract having no Owner, eliminating the ability to call privileged functions.

If renounceOwnership() is called in ArchToken.sol, the ownership of the contract will be transferred to the zero address, rendering the functions containing the onlyOwner modifier unusable:

Similar operations could be performed with revokeRole() or renounceRole() in LvUSDToken.sol, or with revokeAdmin() on any other contract in scope.

It has been observed that no pausing mechanism is implemented across the contracts. However, pausing mechanisms can be helpful in various situations, such as preventing users from interacting with specific functions under certain conditions (such as a contract upgrade or migration) or as a way to avoid fund loss to attackers in the eventuality of a security breach.

It has been detected that some parameter-modifying functions do not have logical limits. This may cause the contract to function with parameter values that, although allowed, make no sense in the application context, which might cause various problems or even render the contract unusable.

Code Location

ParameterStore.sol

    function changeOriginationFeeRate(uint256 newFeeRate) external onlyGovernor {
        emit ParameterChange("originationFeeRate", newFeeRate, _originationFeeRate);
        _originationFeeRate = newFeeRate;
    }

This function's lack of logical limits allows the fee charged on every opened position to be set at any % desired. So, for example, setting it to 0 will mean that no fee is charged on every position opened while setting it to 1 ether will mean that 100% of the OUSD swapped for the borrowed LvUSD will be transferred to the treasury, making the position unwindable since the principle deposited will never be enough to pay for the borrowed LvUSD (unless the position is opened for only 1 cycle).

Greater values will stop the position opening functionality since it will try to transfer more OUSD to the treasury in the concept of fees than the OUSD amount available.

Coordinator.sol

    function _takeOriginationFee(uint256 _leveragedOUSDAmount) internal returns (uint256 fee) {
        uint256 _fee = _paramStore.calculateOriginationFee(_leveragedOUSDAmount);
        _ousd.safeTransfer(_paramStore.getTreasuryAddress(), _fee);
        return _fee;
    }

In addition, changeMaxNumberOfCycles(), changeArchToLevRatio(), and changeCurveMaxExchangeGuard() also lack from parameter limits.

It has been detected that positions with 0 leverage cycles can be opened. Although these kinds of positions make no sense in the context of these contracts, they still can be opened (i.e., because of user input error).

Opening a position like this will have the following consequences:

• Any ARCH sent with the position will be burned and lost forever (as described in the UNUSED ARCH NOT RETURNED WHEN OPENING A POSITION finding).
• No fees will be collected since opening fees are calculated from the leverage obtained, which will be zero in this scenario.
• If a USD rebase takes place after opening the position, the position will remain locked forever. This is caused by the behavior described in the OUSDTOTAL VARIABLE SHOULD BE REMOVED finding. When trying to withdraw any OUSD under the position, oUSDTotal will remain the same amount as the initial OUSD sent as principal, and oUSDAmountToWithdraw will be greater because of the rebase, causing the following required statement never to be fulfilled:

CDPosition.sol

    function withdrawOUSDFromPosition(uint256 nftID, uint256 oUSDAmountToWithdraw) external nftIDMustExist(nftID) nonReentrant onlyExecutive {
        require(_nftCDP[nftID].oUSDTotal >= oUSDAmountToWithdraw, "Insufficient OUSD balance");
        _nftCDP[nftID].oUSDTotal -= oUSDAmountToWithdraw;
    }

Struct CDP is defined within CDPosition.sol contract. It contains information about positions, such as the initial LvUSD amount borrowed, the OUSD shares held by the position, etc.

However, it has been detected that oUSDTotal is declared, used, and never updated, which could cause unpredictable behavior or even render positions unwindable.

According to the comments in the code, this variable holds the total OUSD amount held by the position, including the initial OUSD amount staked (principle), the leverage obtained, and the profits obtained by interest or rebases. This information could also be retrieved using CDP.shares.

However, this variable is never updated, so the oUSDTotal value will be stuck forever with the principle + leverage OUSD amount. Also, this variable is incorrectly used in a require statement to assert that no more OUSD than the available is used:

CDPosition.sol

    function withdrawOUSDFromPosition(uint256 nftID, uint256 oUSDAmountToWithdraw) external nftIDMustExist(nftID) nonReentrant onlyExecutive {
        require(_nftCDP[nftID].oUSDTotal >= oUSDAmountToWithdraw, "Insufficient OUSD balance");
        _nftCDP[nftID].oUSDTotal -= oUSDAmountToWithdraw;
    }

When following the function logic flow, it has been detected that oUSDTotal is compared to oUSDAmountToWithdraw, that is, the OUSD amount remaining under the position after swapping OUSD for LvUSD to repay the leverage obtained. After the comparison, oUSDAmountToWithdraw is subtracted from oUSDTotal, leaving its value inconsistent.

Code Location

CDPosition.sol

    struct CDP {
        uint256 oUSDPrinciple; // Amount of OUSD originally deposited by user
        uint256 oUSDInterestEarned; // Total interest earned (and rebased) so far
        uint256 oUSDTotal; // Principle + OUSD acquired from selling borrowed lvUSD + Interest earned
        uint256 lvUSDBorrowed; // Total lvUSD borrowed under this position
        uint256 shares; // Total vault shares allocated to this position
    }

ParameterStore.sol contract stores global parameters that are used when managing positions, such as _maxNumberOfCycles, _originationFeeRate, etc.

However, it has been detected that parameters with a similar format or that are used in similar ways (like percentages, for example) are defined and used differently, which could cause contract unexpected behavior (or even worse situations such as fund loss) if they are mistakenly modified.

Code Location

ParameterStore.sol

        _maxNumberOfCycles = 10;
        _originationFeeRate = 5 ether / 100;
        _globalCollateralRate = 90;
        _rebaseFeeRate = 10 ether / 100; // meaning 10%
        _treasuryAddress;
        _curveGuardPercentage = 90;
        _slippage = 2; // 2%;
        _archToLevRatio = 1 ether; // meaning 1 arch is equal 1 lvUSD
        _curveMaxExchangeGuard = 50; // meaning we allow exchange with get 50% more then we expected
        _treasuryAddress = address(0);

In this example can be seen how two different rates (with 90% and 10% values, respectively) are calculated differently.

Once any user opens a position, an NFT containing the relative information gets minted to the user. This NFT grants the position ownership and can be burned (if the position is withdrawn) or sold for a profit.

When the NFT is minted using safeMint(), _setApprovalForAll() also gets called, approving the address with EXECUTIVE role in the NFT contract (ideally LeverageEngine contract) to burn or transfer the NFT (needed for burning the NFT once the position gets unwound).

However, it has been detected that, although these NFTs can be easily transferred by calling PositionToken.transferFrom(), the LeverageEngine contract will not be approved to interact with the NFT anymore since the owner is different.

This causes the position to remain locked until one of the two following conditions is met:

• A new approval to LeverageEngine gets issued by the new owner.
• The new owner opens another position, called _setApprovalForAll(), approving LeverageEngine to interact with every NFT owned by the new user.

Unused pieces of code such as modifiers or variables have been found through the code. These unused variables increase gas costs for contract deploying and interactions, impact code readability and might cause the contract to behave in unexpected ways or introduce new vulnerabilities if these variables are mistakenly used.

Code Location

LeverageEngine.sol

    uint256 internal _positionId;

This variable is not used anywhere in the code.

CDPosition.sol

    struct CDP {
        uint256 oUSDPrinciple; // Amount of OUSD originally deposited by user
        uint256 oUSDInterestEarned; // Total interest earned (and rebased) so far
        uint256 oUSDTotal; // Principle + OUSD acquired from selling borrowed lvUSD + Interest earned
        uint256 lvUSDBorrowed; // Total lvUSD borrowed under this position
        uint256 shares; // Total vault shares allocated to this position
    }

oUSDInterestEarned is declared within CDP struct, but it is not used anywhere in the code.

    modifier notImplementedYet() {
        revert("Method not implemented yet");
        _;
    }

This modifier is not used anywhere.

Redundant statements have been found through the code. These statements increase gas costs for contract deploying and interactions, impact code readability.

Code Location

In this example, _treasuryAddress is declared twice but no actual value is assigned:

ParameterStore.sol

    address internal _treasuryAddress;
    uint256 internal _curveGuardPercentage; // in regular (0-100) percentages
    uint256 internal _slippage; // in regular (0-100) percentages
    /// example for _archToLevRatio: If each arch is worth 1000 lvUSD, set this to 1000
    uint256 internal _archToLevRatio;
    // maximum allowed "extra" tokens when exchanging
    uint256 internal _curveMaxExchangeGuard;

    event ParameterChange(string indexed _name, uint256 _newValue, uint256 _oldValue);
    event TreasuryChange(address indexed _newValue, address indexed _oldValue);

    function initialize() public initializer {
        _grantRole(ADMIN_ROLE, _msgSender());
        setGovernor(_msgSender());
        setExecutive(_msgSender());
        setGuardian(_msgSender());

        _maxNumberOfCycles = 10;
        _originationFeeRate = 5 ether / 100;
        _globalCollateralRate = 90;
        _rebaseFeeRate = 10 ether / 100; // meaning 10%
        _treasuryAddress;
        _curveGuardPercentage = 90;
        _slippage = 2; // 2%;
        _archToLevRatio = 1 ether; // meaning 1 arch is equal 1 lvUSD
        _curveMaxExchangeGuard = 50; // meaning we allow exchange with get 50% more then we expected
        _treasuryAddress = address(0);
    }

While comments are useful for understanding true purpose and functionality of the code, misleading comments have been detected that do not match with the actual implementation in the code.

Code Location

In this instance, comments describe a 3% slippage and fee overrun, but it is previously fixed at 1%.

Exchanger.sol

    function _swapOUSDforLvUSD(uint256 amountOUSD, uint256 minRequiredLvUSD) internal returns (uint256 lvUSDReturned, uint256 remainingOUSD) {
        // Estimate "neededOUSD" using get_dy()
        uint256 _needed3CRV = _poolLvUSD3CRV.get_dy(_indexLvUSD, _index3CRV, minRequiredLvUSD);
        uint256 _neededOUSD = _poolOUSD3CRV.get_dy(_index3CRV, _indexOUSD, _needed3CRV);
        uint256 _neededOUSDWithSlippage = (_neededOUSD * 101) / 100;

        require(amountOUSD >= _neededOUSDWithSlippage, "Not enough OUSD for exchange");

        // We lose some $ from fees and slippage
        // multiply _neededOUSD * 103%
        uint256 _returned3CRV = _xOUSDfor3CRV(_neededOUSDWithSlippage);

        uint256 _returnedLvUSD = _x3CRVforLvUSD(_returned3CRV);
        require(_returnedLvUSD >= minRequiredLvUSD, "Not enough LvUSD in pool");

        // calculate remaining OUSD
        remainingOUSD = amountOUSD - _neededOUSDWithSlippage;
        _ousd.safeTransfer(_addressCoordinator, remainingOUSD);

        // send all swapped lvUSD to coordinator
        _lvusd.safeTransfer(_addressCoordinator, _returnedLvUSD);

        return (_returnedLvUSD, remainingOUSD);
    }

In the loop within getAllowedLeverageForPosition() function in ParameterStore.sol contract, the variable i is incremented using i++. It is known that, in loops, using ++i costs less gas per iteration than i++. This also affects variables incremented inside the loop code block.

Code Location

ParameterStore.sol

    function getAllowedLeverageForPosition(uint256 principle, uint256 numberOfCycles) public view returns (uint256) {
        require(numberOfCycles <= _maxNumberOfCycles, "Cycles greater than max allowed");
        uint256 leverageAmount = 0;
        uint256 cyclePrinciple = principle;
        for (uint256 i = 0; i < numberOfCycles; i++) {
            cyclePrinciple = (cyclePrinciple * _globalCollateralRate) / 100;
            leverageAmount += cyclePrinciple;
        }
        return leverageAmount;
    }

Multiple contracts on scope exceed 24576 bytes contract size limit, and could not be effectively deployed on the mainnet.

Code Location

• VaultOUSD.sol: 26656 bytes
• Exchanger.sol: 25194 bytes

Many contracts in scope imports console.sol contract but none of its functions are used anywhere, with the exception of Exchanger.sol.

By default, any EOA holding any OUSD balance will be automatically enrolled in the token's rebase feature, meaning that their balance will be increased whenever the token is rebased without having to perform any additional action.

However, this is not true for any multi-sig wallet or any other kind of smart contract, needing to enroll manually by calling OUSD's rebaseOptIn() function. Otherwise, no OUSD yield will be earned.

Although set in place, contracts such as ReentrancyGuardUpgradeable, UUPSUpgradeable, and AccessControlUpgradeable are not initialized in any of the smart contracts in which they are being used.

Although the current implementation allows these contracts to properly function without being initialized, future upgrades might not behave the same way, creating new vulnerabilities or even rendering the contract unusable.

Certain ERC20 tokens may modify token balances or amounts used in transfer(), transferFrom(), and balanceOf() calls. One of these tokens are deflationary tokens, that charge a certain fee for every transfer()`` ortransferFrom()``. During the code review, it has been detected that the protocol does not support Fee-on-transfer tokens, due mainly to the incompatibilities and balance mismatches that would cause every transaction to revert.

Code Location

Multiple of these mismatches have been already commented on findings above, such as POSITIONS CANNOT BE UNWOUND DUE TO OUSD BEHAVIOR finding.

Several tokens do not revert in case of failure and return false. If that happened, Treasury would not receive the burned ARCH and any user could open positions without having to pay any ARCH.

Although ARCH token implementation currently reverts on failure, it is still considered a best practice to check the return value of any external token interaction. This is being currently done for every interaction with external tokens (such as LvUSD, 3CRV, or OUSD) by using SafeERC20Upgradeable, but not with ARCH. This may lead to contract malfunctions if ARCH implementation changes in a future or during the development phase.

As described in finding CONTRACT DEPENDENCIES CANNOT BE MODIFIED, when setDependencies() function from PoolManager.sol is called, also two approve() calls of LvUSD and 3CRV are performed. However, unlike the rest of the contracts, approve() is used, instead of safeApprove(), safeIncreaseAllowance(), or safeDecreaseAllowance().

As also described in UNCHECKED TRANSFER finding, checking the return value of these kind of transfers is considered a good practice, and could prevent contract malfunctions in case of approval failures if non revert on fail tokens are in use.

In most of the interactions with pools (such as swaps or liquidity providings), slippage % is somehow adjustable (since _slippage parameter from ParameterStore.sol contract is used).

However, in some interactions, such as when providing liquidity using PoolManager.sol's fundPoolWith3CRV(), slippage % is hardcoded, and used as _min_mint_amountvalue.

Code Location

PoolManager.sol

    function fundPoolWith3CRV(address buyerAddress, uint256 amoutToFundInLvUSD) external nonReentrant onlyAdmin returns (uint256) {
        /// Method assumes that this contract , has allowance to spend buyerAddress 3CRV tokens
        /// Method assumes that this contract, has allowance to spend Coordinator lvUSD tokens
        require(_lvusd.balanceOf(_addressCoordinator) > amoutToFundInLvUSD, "Insufficient lvUSD on Coord");
        // // Transfer lvUSD and 3CRV to this contract
        _lvusd.safeTransferFrom(_addressCoordinator, address(this), amoutToFundInLvUSD);
        _crv3.safeTransferFrom(buyerAddress, address(this), amoutToFundInLvUSD);
        uint256[2] memory amounts = [amoutToFundInLvUSD, amoutToFundInLvUSD];
        uint256 expectedTokenAmountToGet = (_poolLvUSD3CRV.calc_token_amount(amounts, true) * 99) / 100;
        return _poolLvUSD3CRV.add_liquidity(amounts, expectedTokenAmountToGet, buyerAddress);
    }

Exchanger.sol

function _swapOUSDforLvUSD(uint256 amountOUSD, uint256 minRequiredLvUSD) internal returns (uint256 lvUSDReturned, uint256 remainingOUSD) {
        // Estimate "neededOUSD" using get_dy()
        uint256 _needed3CRV = _poolLvUSD3CRV.get_dy(_indexLvUSD, _index3CRV, minRequiredLvUSD);
        uint256 _neededOUSD = _poolOUSD3CRV.get_dy(_index3CRV, _indexOUSD, _needed3CRV);
        uint256 _neededOUSDWithSlippage = (_neededOUSD * 101) / 100;

        require(amountOUSD >= _neededOUSDWithSlippage, "Not enough OUSD for exchange");

        // We lose some $ from fees and slippage
        // multiply _neededOUSD * 103%
        uint256 _returned3CRV = _xOUSDfor3CRV(_neededOUSDWithSlippage);

        uint256 _returnedLvUSD = _x3CRVforLvUSD(_returned3CRV);
        require(_returnedLvUSD >= minRequiredLvUSD, "Not enough LvUSD in pool");

        // calculate remaining OUSD
        remainingOUSD = amountOUSD - _neededOUSDWithSlippage;
        _ousd.safeTransfer(_addressCoordinator, remainingOUSD);

        // send all swapped lvUSD to coordinator
        _lvusd.safeTransfer(_addressCoordinator, _returnedLvUSD);

        return (_returnedLvUSD, remainingOUSD);
    }

Some unused functions have been declared in Exchanger.sol contract. Although they are declared as external, they are also declared with the 'OnlyExecutivemodifier, which restricts the usage of these functions only to the address havingExecutiverole, which can only be assigned to one address at a time, and it is supposed to beCoordinator.sol` contract.

However, no calls to these functions have been detected in Coordinator.sol, making these functions useless.

Code Location

PoolManager.sol

function xLvUSDfor3CRV(uint256 amountLvUSD) external nonReentrant onlyExecutive returns (uint256) {
        return _xLvUSDfor3CRV(amountLvUSD);
    }

    function x3CRVforOUSD(uint256 amount3CRV) external nonReentrant onlyExecutive returns (uint256) {
        return _x3CRVforOUSD(amount3CRV);
    }

    function xOUSDfor3CRV(uint256 amountOUSD) external nonReentrant onlyExecutive returns (uint256) {
        return _xOUSDfor3CRV(amountOUSD);
    }

    function x3CRVforLvUSD(uint256 amount3CRV) external nonReentrant onlyExecutive returns (uint256) {
        return _x3CRVforLvUSD(amount3CRV);
    }

In public functions, array arguments are immediately copied to memory, while external functions can read directly from calldata. Reading calldata is cheaper than memory allocation. Public functions need to write the arguments to memory because public functions may be called internally. Internal calls are passed internally by pointers to memory. Thus, the function expects its arguments being located in memory when the compiler generates the code for an internal function.

Code Location

Consider marking below functions as external instead of public if they will never be directly called within the same contract or in any of their descendants:

\textbf{\underline{AccessController.sol}:} addMinter, revokeAdmin, getAddressGovernor, getAddressGuardian

\textbf{\underline{ParameterStore.sol}:} getTreasuryAddress, getCurveGuardPercentage, getSlippage, getArchToLevRatio, getAllowedLeverageForPositionWithArch, calculateOriginationFee, calculateArchNeededForLeverage