File(s) affected: master.fc, master-admin.fc, user.fc, user-admin.fc

Description: The function master-admin::update_full_config_process() replaces the entire master storage, including:

• asset_dynamic_collections, which should not be externally modified. This dictionary holds crucial accounting variables for each whitelisted asset, updated internally through user interactions like supply, borrow, and liquidation. These variables determine withdrawal limits, debt, and yield growth. External changes to this dictionary could prevent users from withdrawing funds or disrupt the system's accounting.
• new_upgrade_info, which can be used to bypass the upgrade process of both contracts master and user.

We also want to highlight the existence of debug code in both contracts master and user that allows the admin to modify asset_dynamic_collections (supply and borrow rates as well as total supplied, total borrowed and token balances) and adjust users’ principals, including resetting debt or supplied amounts.

Recommendation: Consider removing or adapting these functions to reduce the excessive power the admin has over the code of the system and the system’s and users’ balances.

Description: There are three multi-contract flows resulting in state updates that may not be reverted in case of unexpected errors. This could occur at any step except for the initial one.

For the supply flow, 1) assets are transferred to the master contract, 2) the user contract is locked, 3) the master contract verifies if the max supply token is reached, if yes, saves the new balance and 4) sends a success or failure message to the user contract that can update the rewards and accrue the tracking indexes. Here, an unexpected error can happen:

• in step 2) if s_rate == 0 or b_rate == 0, which will create a divide by 0 error in principal_value_supply_calc() or principal_value_supply_calc()
• in step 3) if min_principal_for_rewards == 0 and new_total_supply == 0 or new_total_borrow == 0, which will create a divide by 0 error in supply_success_process().
• in step 3) if the value of forward_ton_amount is higher than the amount of TON available in the contract.
• in step 4) if s_rate == 0 or b_rate == 0, which will create a divide by 0 error in principal_value_supply_calc() or principal_value_supply_calc(). If any of these cases occur, state updates of previous steps will not be reverted, with a possible consequence of assets locked, the user contract remaining in a locked state, the master contract with a new balance even if in the user contract, the rewards are not considered and the tracking indexes are not updated.

For the withdrawal flow, 1) the request is registered in the master contract, 2) the user contract is locked, 3) the correct amount is withdrawn from the balance of the master contract which 4) sends a success or failure message to the user contract that can update the rewards and accrue the tracking indexes. Here, an unexpected error can happen:

• at several steps if s_rate == 0 or b_rate == 0, which will create a divide by 0 error in principal_value_supply_calc() or principal_value_supply_calc().
• in step 3) if the value of forward_ton_amount is higher than the amount of TON available in the contract. If it occurs, here again, state updates of previous steps will not be reverted, leaving the contracts in an inconsistent state.

For the liquidation flow, 1) assets are transferred to the master contract, 2) the user contract is locked for liquidation, and the exact amounts are calculated as well as the principals are updated, 3) the master contract updates its accounting variables and 4) sends a success or failure message to the user contract that can update the rewards and accrue the tracking indexes. Here, an unexpected error can happen:

• in each step where s_rate == 0 or b_rate == 0 and the functions principal_value_supply_calc() or principal_value_supply_calc() are executed, which will create a divide by 0 error.
• in step 2), if collateral_price == 0 in get_collateral_quote() or liquidate_user_process(), which will create a divide by 0 error.
• in step 2), if borrow_amount == 0 in liquidate_user_process(), which will create a divide by 0 error.
• in step 3), if new_collateral_total_supply == 0 or new_collateral_total_borrow == 0 or loan_new_total_supply == 0 or loan_new_total_borrow == 0 in liquidate_satisfied_process(), which will create a divide by 0 error.

While these cases are unlikely with the current codebase (in particular, s_rate == 0 or b_rate == 0), it is better to make sure that explicit errors help users to throw properly and do all necessary state rollbacks so that the system does not remain in an inconsistent state.

Recommendation: Consider making sure that explicit errors let user contracts throw properly and do all necessary state rollbacks so that the system does not remain in an inconsistent state.

File(s) affected: master-admin.fc, master-liquidate.fc

Description: It is important to validate inputs, even if they only come from trusted addresses, to avoid human error that would result in breaking core system invariants:

1. For each asset, the invariant 0 <= collateral_factor <= liquidation_threshold <= 10000 should be preserved;
2. The value of liquidation_bonus should be greater than 10000.
3. The value of liquidation_reserve_factor should be lower than constants::reserve_liquidatoin_scale;
4. The value of reserve_factor should be lower than constants::reserve_scale;
5. It is possible with the function add_new_token_dynamics_process() to override an existing supported token, even if the name of the function suggests the opposite.
6. It may be relevant to add input validation for the values of borrow_rate_slope_low, borrow_rate_slope_high, supply_rate_slope_low, supply_rate_slope_high, target_utilization, origination_fee.
7. It is possible for the admin via the operation op::update_full_config to override the current storage of the master contract without any integrity checks about the old or new variables. This manual operation can be prone to manual error and integrity checks could be added to make sure that the storage cannot be updated into an inconsistent state. For instance, this lack of validation could theoretically disrupt the local storage layout and, therefore, the system accounting, potentially resulting in users losing their funds.
8. In master-liquidate.fc, add a check to revert if a liquidator uses the same asset_id for the collateral and the repaid assets.

Recommendation: Consider adding the relevant checks.

File(s) affected: master-storage.fc

Description: It is possible for the admin via the functions update_dynamics_process(), disable_contract_for_upgrade_process(), force_enable_process() and cancel_upgrade_process() to disable or enable the master contract. If the protocol is temporarily disabled with active positions, these positions could become liquidable and, when the protocol is set back to active, a race condition will happen between borrowers who want to send back more collateral and liquidators who want to liquidate these users, which seems unfair for borrowers who were not able to do that earlier because protocol operations were disabled.

Recommendation: Consider adding a period when the system is activated again to delay the liquidations but make it possible instantly to supply more collateral to favor depositors over liquidators.

File(s) affected: user-liquidate.fc

Description: When a liquidation begins, the state of the user's contract is modified by incrementing its value by one. This action blocks the contract owner from making deposits or withdrawals while allowing others to continue liquidating the user's position. In a highly volatile market, it is possible that the owner could attempt to deposit funds to save their position, but a liquidator might act more quickly and complete the liquidation first while also permitting other users to liquidate blocking the owner from keeping the position.

Recommendation: We recommend allowing deposits and liquidations at the “same time”. In this way, the owner of the user contract can minimize the loss in such scenarios.

File(s) affected: user-utils.fc

Description: A different collateral factor can be used for two different assets. Since in user-utils::calculate_maximum_withdraw_amount() the collateral factor is used on aggregated balances (taking into account several assets), issues could happen if different Collateral Factors are set as described below: The issue resides in the formula to calculate max_amount_to_reclaim. Using a scenario of supplying 100 USD of A, 100 USD of B, and borrowing 65 USD of C:

• CF A = 65%, CF B = 60%;
• aggregated supply balance is 200 USD;
• aggregated borrow balance is 65 USD;
• total amount of C that can be borrowed is 65+60 = 125 USD
• max amount to reclaim for A is 200 - (65/0.65) = 100.
• 100 USD of A is available so the function says the user can withdraw 100 USD of A. After the withdrawal, the supply is only 100 USD of B. Since CF B is 60%, the user cannot borrow more than 60 USD of C. But he has 65 USD of C. So the function user-utils::calculate_maximum_withdraw_amount() returned an amount to withdraw that is too high, because it would make the user over-collateralized.

Recommendation: Consider making sure to correctly calculate the aggregated collateralization of a user by multiplying the correct amount of a given asset with its associated collateral factor.

Description: This issue, reported in the initial report, appeared to be a false positive.

File(s) affected: user.fc

Description: Any address can send an internal message to user sc with op::get_store. As a result, the contract returns the current storage of the contract, and all the balance minus a few TONS reserved for the storage, using the mode sendmode::CARRY_ALL_BALANCE + 2. It could be assumed that only the user of the lending wallet should be able to do this operation, or that at least not all the exceeding TONs are sent to an arbitrary address. The severity is Low since no significant balance is expected in the user contracts.

Recommendation: Consider updating the mode to sendmode::CARRY_ALL_REMAINING_MESSAGE_VALUE.

Description: Only three operations are logged: supply, withdrawal, and liquidation. However, other operations are not logged such as enabling or disabling the master contract, or updating the storage of the master contract (op::update_dynamics, op::update_full_config). Also, tracking the operations op::claim_asset_reserves could help to reconstruct via event monitoring the current accounting of the protocol. This lack of information or events could make harder the monitoring of on-chain operations by off-chain observers such as liquidators or risk management tools. It could also make the identification of a hack attempt unnoticed (ex: updating the oracles parameters).

Recommendation: Consider improving the coverage of core system operations by events wherever relevant.

File(s) affected: utils.fc, user-utils.fc, user-withdrawal.fc, master-admin.fc, asset-dynamics-packer.fc

Description: At several locations in the code, invariant checks may be desired to make sure that unexpected edge cases cannot happen. For instance:

1. In user-utils::calculate_maximum_withdraw_amount(), a check could return 0 or throw if get_avaliable_to_borrow() returns a negative value.
2. In user-withdrawal::withdraw_user_process(), the if condition if (borrow_is_collateralized) could become if (enough_price_data & borrow_is_collateralized) to explicitly only accept this case and do nothing otherwise. If not, updating the code of the called function in future versions may change the assumptions at the level of the calling function, which is the case here.
3. In utils::around_zero_split(), we should check that lower <= upper.
4. In master-admin::submit_upgrade_process(), the function should revert if new_master_code and new_user_code are both null.

The next item was self-identified by the EVAA team during the audit and was added to the report.

5. In asset-dynamics-packer::pack_asset_dynamics(), the function should revert if any of total_supply_principal or total_supply_principal is negative.

Recommendation: Consider checking if relevant core invariants could be added to the code and add them, while making sure that it cannot become a way to DoS the system or to throw without rolling back the state updated during previous contract interactions within a multi-contract flow.

File(s) affected: master-supply.fc, user-utils.fc

Description: The variable dust is used within the system to simplify calculations and give a smoother user experience (getting rid of residuals). However, it is compared with two types of values:

• principals in master-supply::supply_success_process();
• present values in user-utils::calculate_maximum_withdraw_amount();

Recommendation: Consider clarifying with which type of value this variable should be compared.

Description: Race conditions can exist in the system and some concurrent operations in the code could be impacted by this. For instance:

• repay VS liquidate;
• withdraw VS idle contract;
• withdraw VS withdraw due to limited available assets;
• supply VS supply due to the max supply cap of a given asset;

Recommendation: Consider identifying impacted functions and risks should be documented to users.

Description: Any address can lock for a short period the user contract of another user by supplying a minimum amount on its behalf. No clear negative scenario has been identified.

Recommendation: Consider assessing if this should be the expected behavior.

File(s) affected: user-storage.fc, master-storage.fc

Description: The official doc of FunC recommends as follows:

Use `end_parse()` wherever possible when reading data from storage and the message payload. Since TON uses bit streams with variable data format, it’s helpful to ensure that you read as much as you write. This can save you an hour of debugging.".

Some eligible areas of the codebase are not following this recommendation, like for instance in user-storage.fc or master-storage.fc.

Recommendation: Consider using end_parse() wherever relevant, but where it should not introduce a potential source of Denial Of Service.

File(s) affected: user-get-methods.fc

Description: The following getters functions must be called with a valid cell prices_packed to return information about the user smart contract. However, the fact that the cell prices_packed contained invalid data is ignored by the following functions:

1. user-get-methods::getAccountHealth();
2. user-get-methods::getAvailableToBorrow();
3. user-get-methods::getIsLiquidable();
4. user-get-methods::getAggregatedBalances();
5. user-get-methods::get_maximum_withdraw_amount();
6. master-get-methods::getCollateralQuote();

As a result, users will not know if the value returned by these functions is correct. Also, these functions cannot be used to verify if the prices provided by oracles are valid for the system, which is even more relevant for liquidation bots that may want to use these getters to make periodical validity checks of oracle data.

Recommendation: Consider returning two values instead of one: the returned value and the validity of prices_packed.

Description: The following is still unclear:

1. Fees for logs (fee::log_tx) are deduced from msg_value in master_liquidate.fc and master_withdrawal.fc, but not in master_supply.fc.
2. Fees for forwarding a cell (cell_fwd_fee()) are included only in withdraw_min_attachment() but not in similar functions for the supply and liquidate flows.

Recommendation: Consider clarifying if this works as expected or not.

Description: The system uses prices used by whitelisted oracles to calculate the current amounts of assets used by the protocol in specific denominations. This leads to several risks:

1. Extreme prices can be provided due to an error, a price manipulation attack, or extreme market conditions;
2. The number of decimals used can differ between different oracles;
3. An oracle can become unavailable. If the number of available oracles goes below the current threshold, no more operations using prices can work until these oracles are available again.

Recommendation: Consider assessing these risks and defining measures to reduce the likelihood of these scenarios.

File(s) affected: master-supply.fc, master-liquidate.fc

Description: In the function master-liquidate::master_core_logic_liquidate_asset_unchecked(), we have the following comment:

;; note that the asset balance does NOT increase at this point (unlike in Supply)
;; because Supply always succeeds, the received assets might be used in other operations immediately
;; but liquidate might not succeed - in which case it should refund the assets - thus these assets can't be made available yet

However, in the function master-supply::master-core-logic-supply-asset-unchecked(), we have the following comment:

;; got money, but we don't update token_balance yet
;; that is because supply_fail is possible,
;; so we might need to refund money-received later
;; and we gotta make sure this money is available
;; (so they can't be used meanwhile until supply_success is confirmed)

These two comments are contradictory.

Finally, a third comment is incorrect, in supply_success_process(), when the token supply cap is reached:

;;revert principals on user sc

Recommendation: Consider aligning the comments and the code for all three items.

File(s) affected: user-utils.fc

Description: Supported assets can be deprecated temporarily or forever. Such operation would be made at the level of the master contract. However, it could negatively impact methods at the level of the user contract where non-zero amounts could remain in the dict user_principals for deprecated keys. For instance, since all keys in user_principals are browsed, we could have in:

1. account_health_calc() a situation where the function returns false if one of the keys is not found in the dict prices_packed.
2. check_not_in_debt_at_all() a situation where the function returns false even if it is due to a debt in a deprecated token.
3. is_liquidatable() a situation where the function returns false if one of the keys is not found in the dict prices_packed, or where the function incorrectly considers the principal of a non-supported asset for the calculation of the liquidability of the account.
4. get_avaliable_to_borrow() a situation where the function returns false if one of the keys is not found in the dict prices_packed, or where the function incorrectly considers the principal of a non-supported asset for the calculation of the amount available to borrow.
5. get_agregated_balances() a situation where the function returns false if one of the keys is not found in the dict prices_packed, or where the function incorrectly considers the principal of a non-supported asset for the calculation of the aggregated balances.

Recommendation: Consider clarifying if all the items described above are working as expected or if the code should be updated. An option could be to send from the master contract the list of assets currently supported, so these 5 functions could ignore non-supported tokens. However, it should still be possible for users to withdraw deprecated assets. A clear plan should be defined to handle actions allowed for users when an asset is no longer supported in such a way that the solvency of the protocol cannot be affected.

Description: Muldiv operations round down by default. This could be wrong if it is not rounded in the interest of the protocol. For instance, amounts sent to the protocol should be rounded up, and the amount removed from the protocol should be rounded down. Also, depending on the context, computations done in present_value_calc() may require an opposite rounding direction depending on whether it corresponds to the present value of a borrowed amount or of a supplied amount. These particular operations could also be replaced by an explicit call to muldiv(). This can be particularly the case for nested muldiv() operations that can result in a loss of precision.

Recommendation: Consider adopting muldiv() operations wherever required (for instance in present_value_calc()), and adapting them to explicitly use the correct rounding direction.

Description: As stated in the official documentation of TON:

The `impure` specifier means that the function can have some side effects which cannot be ignored. For example, we should put an `impure` specifier if the function can modify contract storage, send messages, or throw an exception when some data is invalid and the function is intended to validate this data.
If impure is not specified and the result of the function call is not used, then the FunC compiler may and will delete this function call.

We identified that some functions in the codebase can revert but do not have an impure specifier. For instance:

• master-withdrawal::master_core_logic_withdraw() where there is a throw_unless() instruction;
• universal-dict::upgrade_storage:get!() where there is a throw_unless() instruction;
• ton::cell_fwd_fee() where there is a throw_unless() instruction;

Recommendation: Consider checking functions that can send messages, update the state, and throw (with a throw statement) or hard revert (due to an error) and add to them an impure specifier.

File(s) affected: master.fc, user-withdrawal.fc

Description: The supply and borrow rates are updated at the beginning of all operations but only for the asset involved in the supply, borrow, or liquidation. Subsequent operations loop over all the user's assets to determine how much to borrow or if the user is liquidatable. However, these operations might use outdated rates, potentially leading to unexpected scenarios, such as a liquidatable position not being detected until it has already accrued bad debt.
For instance, in the liquidation flow, the rates are not updated before being passed to the user contract and, consequently, can be lower than expected by the liquidator. This may result in a failed liquidation since the liquidator is incentivized to liquidate the maximum possible amount, which may then exceed the maximum calculated by the user smart contract due to the lower rates.
Also, in the withdrawal process, within master-withdrawal::master_core_logic_withdraw(), the new supply and borrow rates for the withdrawn asset are calculated and sent to the user contract along with the outdated dynamic collections. When handling a full withdrawal, the function calculate_maximum_withdraw_amount() calculates the maximum amount that can be withdrawn, but it uses the old supply and borrowing rates from the outdated dynamic collections instead of the new rates to calculate old_present_value. Since the rates are updated only when a user interacts with a specific asset, there might be cases where there has not been any interaction for a significant time duration for the withdrawn asset. There could be two scenarios here:

1. The rates for the withdrawn asset are very old: assuming the best scenario where the user has no debt, then the maximum amount would be a very outdated old_present_value.
2. The rates for the other assets the user holds are very old: assuming the user has some debt, then the function get_agregated_balances() would return outdated balances (both positive and negative) which would also impact the correct amount the user should be able to withdraw.

Recommendation: We recommend checking the last time the rates were updated and update them if necessary.

File(s) affected: master.fc

Description: After the master contract is updated, some configuration variables remain uninitialized. A subsequent transaction is expected to initialize them. However, the codebase does not verify whether these variables have been initialized before use. This oversight could lead to unexpected consequences in the accounting of unconfigured assets and potentially impact other funds, or even throw unexpected errors as described in EVAA-2.

Recommendation: Consider adding some form of check that each asset’s config both static and dynamic has been initialized.

Description: Here is a non-exhaustive list of items found that express that the codebase might not be production ready yet:

1. In errors.fc, comment includes ";; NOTE: !!! I don't like this name: because of soft-checks, there are various reasons min_collateral might not be satisfied. Rename to "low_reward" or something similar"
2. In fees.fc, comments ";; todo: measure and set values closer to practical usage", ";; todo: Think more about it" and ";; todo find exact value";
3. In master-storage.fc, comment includes ";;todo rm wallet_to_master everywhere";
4. In liquidate-message.fc, comments ";; todo: !!!! What info do the liquidators need in the reports?" and ";; todo: maybe query_id?";
5. In master.fc, comment indicating: ;; ----------- todo: remove all of admin function below before opensource !!!;
6. In user-upgrade.fc, a part of the function upgrade_user_process() contains commented code.
7. In master-withdrawal.fc, the function ~dump() is used;

Recommendation: Consider clarifying whether the Todos and the commented-out codes are necessary to keep or fix. However, since the codebase is planned to be further developed in the future, consider also adopting a versioning notation in the comments to remove any ambiguity and make sure that comments in the current code match the latest version of the code.

File(s) affected: user-liquidate.fc

Description: The liquidate process defines a maximum collateral reward capped at 50% of the available collateral of the liquidated user for cases without bad debt. However, this threshold can be exceeded by initiating a second liquidation process if the user remains liquidatable. Therefore, a liquidator is incentivized to transfer assets amounting to slightly less than required, keeping the position unhealthy. If both liquidations are well coordinated the user cannot supply assets in-between, since his contract is locked for liquidations.

Exploit Scenario:

1. Assume a collateral factor of 60% and a liquidation threshold of 70% for all assets.
2. A user has a liquidatable position, having 100$ worth of collateral asset A and borrowing 80$ worth of asset B.
3. The liquidator could repay 50% of the collateral, returning the position to a healthy state. However, he is more incentivized to limit this first operation to 30$, leaving the user with 70$ of collateral and a loan of 50$ and allowing another liquidation.
4. The second liquidation targets 50% of the collateral, leaving the user with 35$ worth of collateral overall, instead of 50$. Note: this simplified example does not take the liquidation bonus into account.

Recommendation: Consider allowing a user to supply collateral during ongoing liquidation processes.

File(s) affected: master-supply.fc

Description: This issue was self-identified by the EVAA team during the audit and was added to the report. As such, its severity is Undetermined.

Tracking indexes should be calculated based on old total supply / total borrow. However, the calculation is done based on new_total_supply on line 230 in master-supply.fc.

Recommendation: Calculate the tracking indexes based on the old total supply / total borrow.

File(s) affected: master-supply.fc

Description: This issue was self-identified by the EVAA team during the audit and was added to the report. As such, its severity is Undetermined.

If a user makes a repayment and the max cap is lower than the current total supply, the code (on line 199 in master-supply.fc) will not let the user make a repayment because new_total_supply (which is just the old total_supply_principal + 0) will always be greater than max_token_amount.

Recommendation: Consider only preventing the new supply to go beyond the max cap, not the repaid amount.

File(s) affected: master-admin.fc

Description: This issue was self-identified by the EVAA team during the audit and was added to the report. As such, its severity is Undetermined.

Global tracking indexes tracking_supply_index, tracking_borrow_index and last_tracking_accrualshould be updated during a configuration upgrade if the tracking speed is updated.

Recommendation: Consider updating the global tracking indexes tracking_supply_index, tracking_borrow_index and last_tracking_accrual during a configuration upgrade if the tracking speed is updated.

File(s) affected: master.fc

Description: This issue was self-identified by the EVAA team during the audit and was added to the report. As such, its severity is Undetermined.

There are insufficient input validations on each message coming from user wallet (checks of addresses & custom_response_payload).

Recommendation: Consider checking the validity of addresses and custom_response_payload on each message coming from user wallet.

File(s) affected: master-revert-call.fc

Description: This issue was self-identified by the EVAA team during the audit and was added to the report. As such, its severity is Undetermined.

In the contract master-revert-call.fc, function revert_call_process(), the slice in_msg_body is parsed instead of revert_body when revert_op == op::supply_user or revert_op == op:: liquidate_user or revert_op == op::idle_user.

Recommendation: Consider parsing the slice revert_body instead of in_msg_body.

File(s) affected: master-admin.fc

Description: This issue was self-identified by the EVAA team during the audit and was added to the report. As such, its severity is Undetermined.

In master-admin.fc, the cumulated usage of the functions add_new_token_dynamics_process(), update_master_lm_indexes() and do_data_checks() can lead to a revert error when a token is added to the dict asset_config_collection before being added to the dict asset_dyamics_collection.

Recommendation: Consider adding in the same function a new token to these two collections.

File(s) affected: master-supply.fc, user-supply.fc

Description: This issue was self-identified by the EVAA team during the audit and was added to the report. As such, its severity is Undetermined.

Due to the asynchronous nature of TON, sending multiple supply requests simultaneously could lead to overriding the maximum supply cap check. Supply requests submitted to the master contract are processed one by one, before any consequential success, failure, or revert. As a result, the total supply value used in the user contract to enforce the maximum supply cap check only considers the updated total supply resulting from this single supply request, instead of considering all pending supply requests. This way, a malicious actor can supply more than the maximum supply cap and use it to borrow all the available assets.

Recommendation: Consider virtually accumulating into a variable of the master contract the amount of all pending supply requests, and passing that amount to the user contract. It will let the max supply cap check consider the scenario where all pending supply requests are successful. Then, deduce from this variable the virtual amount when the supply request either succeeds, fails, or reverts.