Vine Money

The Vine Money protocol emphasizes privacy in users' financial activities, achieving it to a certain extent. However, since the team did not provide us a detailed list of privacy promises or an adversarial model description, we refrain from categorizing privacy-related findings as issues or determining their severity. Nevertheless, we summarize here what is kept private, what is not, and the assumptions ensuring this privacy:

• Privacy Enhancements for vUSD:
  vUSD, the stablecoin minted by the protocol, is an ERC20 token with significant privacy enhancements. Messages are not emitted on transfers, mints, burns, or approvals, allowing transaction confidentiality.

Read access to the balance of an address is restricted to the address itself and designated lookers, set by the protocol owner.Lookers are intended to be contracts needing balances for accounting purposes, ensuring privacy unless the owner acts dishonestly (refer to related issues N1 and H1).

• Public Information about vUSD:
  Allowance from any owner to any spender is visible to any address, potentially indicating a relationship between the owner and spender. While allowances and balances are not directly correlated, non-zero allowances can imply such relationships.

• Trove Data Access:
  Getter functions providing data about a trove (debt, collateral, stake) are accessible only to the trove owner or designated lookers (refer again to issue N1).

• Public Total Status of Trove Managers:
  Variables concerning the total status of a trove manager, such as total collateral, total debt, total stakes, and total rewards, are publicly accessible. Adversaries can indirectly infer information about troves by monitoring these global variables and interactions with the protocol. For instance, observing changes in total system collateral before and after an address interacts with the protocol reveals collateral movements.

• Info about stability pool LPs are public:
  The interactions of LPs with the stability pool are public. Messages are emitted on deposits and withdrawals and there are public mappings from every address to ist deposited collateral and rewards.

• Collateral info can be retrieved from the sorted list of the troves:
  The findInsertPosition function within the SortedTroves contract receives a collateral ratio and calculates the addresses of two consecutive troves in the ordered list, with collateral ratios lower and higher than the provided ratio. This external function is accessible to any caller.Calling this function multiple times with various collateral ratios could enable an individual to estimate the collateral ratios of active troves. By observing the changes in the addresses returned by findInsertPosition for different input ratios, an attacker could deduce information about the distribution of collateral ratios among active troves.

• Information leak through side channels:
  Side channels, like gas costs, can reveal info about a user's interaction with the protocol i.e. if the tx involved one branch or another. These attacks may be mitigated by gas padding (supported by Sapphire), but Vine Money protocol lacks measures against such attacks.

The VineSignature::authenticated modifier controls the access to certain getter functions providing information about trove data, such as debt, collateral and stake or the vUSD balance of an address. Its purpose is to enable third parties to access trove information by providing a signed message from the trove owner. However, the current implementation is flawed. The modifier merely verifies that the address included in the signed message (auth.user) matches the address that signed the message.

modifier authenticated(SignIn calldata auth)    {
        // Must be signed within 24 hours ago.
        require( auth.time > (block.timestamp - (60*60*24)) );

        // Validate EIP-712 sign-in authentication.
        bytes32 authdataDigest = keccak256(abi.encodePacked(
            "\x19\x01",
            DOMAIN_SEPARATOR,
            keccak256(abi.encode(
                SIGNIN_TYPEHASH,
                auth.user,
                auth.time
            ))
        ));

        address recovered_address = ecrecover(
            authdataDigest, uint8(auth.rsv.v), auth.rsv.r, auth.rsv.s);

        require( auth.user == recovered_address, "Invalid Sign-In" );

        _;
    }

Nevertheless, there is no enforced relationship between this address and the address for which we seek information.

Consider for example the TroveManagerGetters::getTrove function.

function getTrove(SignIn calldata auth, address _troveManager, address _borrower) external authenticated(auth) view returns (
            uint256 debt,
            uint256 coll,
            uint256 stake,
            uint8 status,
            uint128 arrayIndex,
            uint256 activeInterestIndex
        ) {//Dedaub: auth and _borrower are independent
        return ITroveManager(_troveManager).getTrove(_borrower);ITroveManager.getTrove
    }

Anyone can sign a message with their own address in the auth.user field and invoke getTrove for any borrower. The modifier check will succeed, granting the third party unrestricted access to the borrower's trove information.

In the TroveManager::redeemCollateral function, there's a requirement to verify that the caller possesses a sufficient balance of debt tokens. This verification involves checking if the caller's debt token balance is greater than or equal to the amount they intend to redeem. To perform this check, the function invokes checkBalanceOf from the debt token contract, passing the msg.sender address as an argument. However, checkBalanceOf expects arguments in the form of a SignIn structure (or (SignIn calldata auth, address account) before the fix related to issue H1). Consequently, the call will fail.

There are two possible solutions of this issue:

• Add an additional argument SignIn auth to the redeemCollateral function and invoke checkBalanceOf with this structure.

• Instead of using checkBalanceOf, directly call balanceOf since there's no privacy compromise in this context. The TroveManager making the external call is a looker, and the balance being checked is that of the caller of redeemCollateral.

In the StabilityPool::claimCollateralGains function, collateral gains for specified collateral indices are computed and sent to a Stability Pool liquidity provider. The function utilizes the safeTransfer function to send tokens, assuming that the collateral token is an ERC20 token.

function claimCollateralGains(address recipient, uint256[] calldata collateralIndexes) public virtual {
        _accrueDepositorCollateralGain(msg.sender);

        uint256 loopEnd = collateralIndexes.length;
        uint256[] memory collateralGains = new uint256[](collateralTokens.length);

        uint80[256] storage depositorGains = collateralGainsByDepositor[msg.sender];
        for (uint256 i; i < loopEnd; ) {
            uint256 collateralIndex = collateralIndexes[i];
            uint256 gains = depositorGains[collateralIndex];
            if (gains > 0) {
                collateralGains[collateralIndex] = gains;
                depositorGains[collateralIndex] = 0;
                collateralTokens[collateralIndex].safeTransfer(recipient, gains); //Dedaub: it is assumed that the collateral is an ERC20, although it could be NaT
            }
            unchecked {
                ++i;
            }
        }
        emit CollateralGainWithdrawn(msg.sender, collateralGains);
    }

However, there's a possibility that one of the specified collateral indices corresponds to the native collateral token (ROSE). In such cases, the transfer cannot be completed using safeTransfer, and the call method should be employed instead.

In the Vine Money protocol, the algorithm for updating prices in the price feed encounters difficulties recovering after a significant price change. The process begins with a call to the Band oracle, followed by checks to ensure the oracle's functionality, including successful retrieval of the price, non-zero return, and absence of staleness. If these checks fail, the function reverts. If successful, the oracle price is compared with the stored price in the contract, with the relative difference capped by a maximum deviation.

function _processFeedResponses(
        address _token,
        OracleRecord memory oracle,
        FeedResponse memory _currResponse,
        PriceRecord memory priceRecord
    ) internal returns (uint256) {
        bool isValidResponse = _isFeedWorking(_currResponse, oracle.heartbeat) &&            !_isPriceChangeAboveMaxDeviation(_currResponse, priceRecord);//Dedaub:the oracle response is compared to the stored price, but this is not an accurate test for the validity of the oracle price, especially if a long time has passed since the last time a price was stored in the contract
      . . .
    }

Unlike other protocols such as Prisma or Liquity, which utilize Chainlink oracles and compare prices between consecutive rounds, Vine Money compares the current oracle price with the stored price. While this approach is valid under normal circumstances, it lacks the ability to sufficiently gauge the validity of the oracle price. If a considerable amount of time has elapsed since the last interaction with the protocol, the difference between these two prices could be substantial without indicating an issue with the oracle price.

Although the maximum deviation is set to a large percentage (e.g., 50%), it is assumed that users will interact with the protocol before this threshold is exceeded. However, if this assumption fails, and the oracle price changes significantly, the protocol may become difficult to revive. For instance, if the oracle price changes by more than 50% compared to the stored price, and enough time has passed rendering the stored price stale, all checks in the fetchPrice function will fail, causing transactions to revert. If collateral prices stabilize around this new oracle price, the fetchPrice function will continue to revert, presenting a contrast with the behavior of protocols like Prisma or Liquity.

To address this vulnerability we suggest not just comparing the deviation of the oracle and stored prices, but also consider the time since the last update. If this is large then the comparison makes no sense and should be dismissed and probably the best practice in this case is to use the oracle price.

The contracts Factory, StabilityPool, DebtToken, VineToken and IncentiveVoting contain a setInitialParameters function intended to initialize critical variables for each respective contract. While these functions can be called only once, there exists no access control mechanism, allowing anyone to call them and set vital protocol variables at their discretion. Although it's anticipated that the protocol owner will execute these functions promptly post-deployment, such enforcement is absent from the code. Given the absence of benefits associated with leaving these parameters uninitialized, and considering the immutable nature of the variables defined in the setInitialParameters functions, we recommend declaring these variables as immutable and initializing them within the constructors of the contracts for improved security and control.

Vine contracts currently use, in the BorrowerOperations::sendROSE, the .transfer method for native token (ROSE) transactions. This method forwards a fixed amount of gas (2300) and that was considered to be a measure against reentrancy. However, this approach assumed constant gas costs, which is not the case, since gas costs are subject to change. Any contract using .transfer takes a hard dependency on gas costs and could break after a future gas costs update. Moreover, the use of .transfer hampers interactions with other protocols that require multiple actions or adjustments to accounting variables upon receiving tokens from Vine. Additionally, it poses a challenge for Vine's interaction with accounts using account abstraction.

Since Vine already employs the checks-effects-interactions pattern for security, we believe that it is safe to replace .transfer with .call. This switch would not only eliminate gas cost dependencies but also improve interactions with other protocols.

The vUSD contract allows read access to vUSD balances for addresses designated as lookers, primarily intended for protocol contracts requiring balance access for accounting purposes. However, the setLookers function, controlled by the contract owner, enables arbitrary designation of looker addresses. While it's assumed that only protocol contracts necessitating balance access will be designated as lookers, the absence of restrictions permits the owner to designate any address, potentially compromising the privacy of vUSD holders. The centralization concerns may be mitigated if the contract owner is a DAO, as expected.

The Vine Money protocol includes functions related to bridging Vine Tokens to other chains using the Celer Interchain protocol. However, the integration remains incomplete. Interacting with Celer requires deploying specific contracts with specified functions to both sides of the bridge. Unfortunately, these contracts are absent from the provided codebase for this audit.

The team has communicated that they do not have immediate plans to integrate with Celer or offer cross-chain functionality in the near future. Consequently, we recommend removing the functions setCelerEndpoint, receiveFromChain and burn. Otherwise, the owner can designate any address as a celerEndpoint, allowing them to mint or burn Vine Tokens from any address without constraints.

Within the Vine Money protocol, the owner of the factory contract possesses the authority to enable and disable trove managers. While enabling trove managers aligns with operational needs, the ability to disable trove managers raises concerns.

When the owner decides to disable a trove manager, no conditions are enforced regarding the status of its open troves.

When a trove manager is disabled, critical functions such as burn and mint of the debt token become inaccessible. Consequently, trove owners are unable to perform essential actions such as closing their troves fully or partially, vUSD holders cannot redeem their debt tokens for collateral.

The VineSignature.sol contract is employed to verify message signatures in accordance with EIP-712. However, despite the message to be signed containing only the first two variables of the SignIn struct (address user and uint32 time), the computation of typeHash utilizes the name of the SignIn structure. To enhance compatibility with the EIP, we suggest defining a new structure containing solely these two variables (address user, uint32 time) and employing the name of this new structure in the calculation of typeHash.

In BorrowerOperations.sol, several functions enable users to deposit collateral on their troves: openTrove, addColl and adjustTrove. These functions are payable to accommodate the use of ROSE (the native token of the Oasis network) as collateral. To prevent user errors and ensure safety, it is advisable to implement additional sanity checks. Specifically, it is proposed to verify that when troveManagersData[troveManager].collateralToken is not equal to IERC20(ROSE), the msg.value is zero. This validation ensures that transactions reverting when users attempt to send ROSE to a trove manager with non-ROSE collateral, thus preventing potential loss of funds..

In PriceFeed.sol, there are two instances of unnecessary typecasting:

• In _isPriceChangeAboveMaxDeviation, _currResponse.rate is already of type uint256.
• In _fetchCurrentFeedResponse, _oracle.bandRecord is already of type IStdReference (and not an address), therefore, the casting IStdReference(_oracle.bandRecord) is not needed.

The common implementations of the ERC20 contain two versions of the internal function _approve. The first version accepts as arguments (address owner, address spender, and uint256 value), while the second version includes an additional boolean argument. This boolean parameter indicates whether a message should be emitted or not. In the implementation of the ERC20 standard by the Vine Money team, no messages regarding approvals are emitted. Consequently, the boolean argument is redundant and should be removed. As a result of this modification, the two _approve functions will become identical, necessitating the removal of one of them to eliminate redundancy.

In the PriceFeed contract's _processFeedResponses function, a boolean variable isValidResponse is computed based on two conditions:

bool isValidResponse = _isFeedWorking(_currResponse, oracle.heartbeat) &&//Dedaub: //whenever _processFeedResponses is called this is always true and can be removed            !_isPriceChangeAboveMaxDeviation(_currResponse, priceRecord);

However, _processFeedResponse is only called by fetchPrice if _isFeedWorking(_currResponse, oracle.heartbeat) is true. Therefore, the first term in the isValidResponse calculation, _isFeedWorking(_currResponse, oracle.heartbeat), is redundant

The PriceFeed contract utilizes the Band Oracle, which stores USD prices for various assets along with their corresponding update times. When fetching a price from the oracle, both the base and quote assets must be specified, with the oracle returning the ratio of the base to the quote USD prices and the last update times of these two assets. The last update time of USD is always the timestamp of the block it was called.

In the PriceFeed contract, when a price is stored as the last update time in the contract, the base last update time is used (assuming the quote asset is USD). However, during the computation of _isFeedWorking, both the base and quote update times are checked. If the quote asset is USD as expected, the staleness condition regarding it becomes redundant.

The comparison _currentResponse.success == true can be rewritten as _currentResponse.success.

The address for ROSE in BorrowerOperations.sol is declared as immutable, even though it is not set in the constructor. It could be declared as a constant instead.

The code has the compile pragma ^0.8.19. For deployment, we recommend no floating pragmas, i.e., a fixed version, for predictability. Solc versions ^0.8.19 have some known bugs, which we do not believe to affect the correctness of the contracts.