Borrowing and Lending Platform

In the write function, if the cardinality is less than the size then the cardinality is always incremented, regardless of if a new index was written to or not.

This perturbs the checking of the oldest snapshot on line 68 in the find function as well as everything in the search function.

Only increment the cardinality when a new index is written to in the write function.

Ambit Team: The issue was resolved in commit ac233d2.

Proof of concept: PoC

The functions supply() and withdraw() call accrueRewards() before mutating the points balance of a user. burnTokens() does not which allows someone to claim an abnormally large portion of the rewards by abusing their rewardsIndex and an artificially high balance of points that did not get accrued when incremented.

This allows such a user to burn AMBT just before they withdraw and receive much more rewards than they should. This will also brick the reward claim process for other users as the reward token balance of the contract will not be enough to cover for their rewards.

Call accrueRewards() before mutating the user's balance2 in burnTokens().

Ambit Team: The issue was resolved in commit 277fba3.

Proof of concept: PoC

When a user burns their Ambit, they are immediately credited with points that experience a 5x multiplier. However, if a user burns less than 1e7 of an Ambit, the total.points will not increment: total.points += LoyaltyLib.boostMul(amount.toUint64(), LoyaltyLib.BURN_BOOST);

This is because in the boostMul function, the returned point value will be truncated to 0: points / POINT_DENOMINATOR * boost / DEFAULT_BOOST; A user could maliciously take advantage of this by initially burning more than 1e7 Ambit. This will be reflected in both the total and the user’s point balances as there will not be truncation.

The user can then burn less than 1e7 Ambit which will not be reflected in the total of the Loyalty due to the truncation, but it will be reflected in the point balance of the user because the user’s balance2 is already above 1e7: points.balance2 / POINT_DENOMINATOR * BURN_BOOST / DEFAULT_BOOST;

Because the total points are less than needed, the tokenReward.index will be larger which will inflate the rewards to be dispersed: tokenReward.index += ((amount * REWARDS_MULTIPLIER) / loyalty.getTotalPoints()).toUint128();

Users will not be able to claim their rewards as more reward tokens are attempted to be sent than actually exist in the contract. This can be repeatedly done by malicious users to widen the spread between the total points and sum of all users’ points.

In function burnTokens, remove the user’s prior points from the total and add their new points with the boost similar to function updateBoost

Similarly adjust functions withdraw and claimPoints to avoid the discrepancy as well.

Ambit Team: The issue was resolved in commits a414931 and d4b3fa3.

In the liquidateVaultToken function it is assumed that the totalAmount is always available for withdrawal from the vault since in many cases it would have been previously repaid to the vault in the MarketLiquidation.settleLiquidation function.

However in the case where all funds are actively being borrowed from the vault and a small account is liquidated while the totalAmount is greater than the liabilities, there is not enough USDT to redeem from the vault and the liquidation will revert on line 397 in the DepositorVault.withdrawInternal function.

Ensure the totalAmount is capped at the liabilities earlier on in the liquidation logic, this way there is always guaranteed to be enough USDT to redeem from the vault in the event that the user’s collateral is the vault token.

Additionally, do not allow withdrawals from the DepositorVault to go below the available balance returned by getAvailableBalance. This way it is nontrivial to enter a scenario where there is no USDT left in the vault.

Ambit Team: The issue was resolved in commit a9d6785.

The snapshot system was implemented in Ambit V2 to prevent the manipulation of the utilization rate through instantaneous balance changes in the Depositor Vault. However, the system only examines the latest snapshot, which can be trivially updated because the function takeSnapshot is public.

A user can take a snapshot right after depositing in the vault, and now the snapshot will reflect the current balance. Consequently, a user can still instantly manipulate the utilization rate at will through their deposits and withdrawals and take a snapshot right after.

Modify the visibility of function of takeSnapshot to private. Furthermore, take a snapshot upon borrowing and repaying as the total liabilities are modified.

Consider taking a weighted average of the assets available across the past 4-10 snapshots so it is less prone to instantaneous manipulation. Also, consider utilizing a minimum deposit and borrow amount as well to limit a user from trivially creating many snapshots and pushing the 4-10 snapshots back.

Ambit Team: The issue was resolved in commit 8530b71.

Proof of concept: PoC

The UserPoints.boost is supposed to be at 10 (equating to 1x) but is 0 initially. When claimPoints() gets called after waiting for 2 EPOCHs the UserPoints.boost is set in the following snippet: user.boost = user.boostOrDefault(); // @audit gets set from 0 to 10 here

The user can make their boost larger by calling claimBoost() if one is available to them. The current implementation of the system has a FirstLoanBoostModule which gives users an additional 0.1x on top of their 1x boost. The requirement for it is that they have borrowed more than 0 from the protocol and that they have a positive point balance through UserPoints.total().

The issue arises if the user burns AMBT with burnTokens() before calling claimPoints() to set their initial UserPoints.boost to 10 and then calls claimBoost(). This would set their boost to 0.1x instead of to 1x due to the following LoC: user.boost = Math.min(user.boost + boost, LoyaltyLib.MAX_BOOST).toUint64();

Here UserPoints.boost is not yet set to the default boost, hence the user's boost for their balance1 becomes 10x smaller than it should be and causes them to lose out on 90% of the rewards they should be receiving.

Consider implementing the following changes across the Loyalty.sol file:

1. Check whether the UserPoints.balance1 specifically is > 0 for boost eligibility.
2. Set the UserPoints.boost to UserPoints.boostOrDefault() in burnTokens() and supply().

Ambit Team: The issue was resolved in commit 8c37f44.

Proof of concept: PoC

Because each token has a different LTV, a liquidation in a specific token produces a change in borrowing power that is not proportionate to the change in liabilities. As a result, it is easily possible for a position to have a lower health score (become unhealthier) post-liquidation.

The position would require more than 1 liquidation at 50% of the liabilities until the health score increased. Consequently, bad debt can stay in the system for a prolonged period and put the protocol at risk of supporting an insolvent position.

• 1000 USDT in liabilities
• 6 ETH each at $100 for 100% LTV ($600 borrowing power)
• 1 BTC each at $400 for 50% LTV ($200 borrowing power)
• Health Factor: ($600 + ($400 * 0.5)) / $1000 = 0.80

Because ETH is the largest position in the portfolio, the ETH will be liquidated.

• 50% of 1000 USDT (liabilities) = 500 USDT
• Proportionate supply = 500 USDT / $100 (ETH price) = 5 ETH
• 500 USDT in liabilities
• 1 ETH in the portfolio ($100 borrowing power)
• 1 BTC in the portfolio ($200 borrowing power)
• New Health Factor: ($100 + ($400 * 0.5)) / $500 = $300 / $500 = 0.60

Prior to liquidating a position, simulate the health factor post-liquidation. Carefully select the asset to liquidate which will ultimately increase the health factor. Furthermore, consider supporting full liability liquidations.

Although this may add complexity as multiple swaps in the portfolio may be needed due to the different supply tokens, it will ensure the borrow position is healthier post-liquidation.

Ambit Team: The issue was resolved in commit 2b4c71d. 24

During the liquidation of a position involving a different ERC-20 token, a swap occurs. In this swap, the minAmountOut is calculated to ensure a minimum amount of tokens is received. However, the estimateSellAmount function calculates the minAmountOut based on the notional value, by multiplying the amount (amount1) by the price (amount2):

uint256 minAmountOut = amount1.mulDiv(amount2, 10 ** decimals);

Changes in the asset's price can lead to minAmountOut being either larger or smaller than the input amount. If the price rises, the minAmountOut may exceed the input, causing the swap and liquidation attempts to fail. Conversely, if the price falls, minAmountOut may be much lower than intended, enabling swaps to exceed the intended max slippage.

Other areas where USDT is assumed to be $1 is the liabilities. Because the liabilities are used in calculations such as an account’s health factor, an account may appear unhealthy when it is not and vice versa.

Modify the calculation for minAmountOut in the estimateSellAmount function to be based on the expected token amount rather than the notional value. This adjustment ensures more accurate and reliable swaps during liquidation. Furthermore, use a USDT price feed to retrieve the market price.

Ambit Team: Because we are a lending protocol that only lends out a single asset, the liabilties and borrow limit are denominated in that base asset, in this case USDT. Additionally, all price oracles are based on USD.

findLargestPosition finds the largest position in a user's portfolio and returns it to be liquidated. The issue arises due to it using the ordinary prices from the oracle instead of the ones with applied discounts when calculating position size.

This will cause some positions, which are normally of higher value but have a higher discount to still be selected over ones with a lower value and a lower discount that equates to them being worth more when liquidating: (, USD[] memory totals) = portfolio.getPortfolioValue(portfolioAssets);

Consider using the discounted prices to find the position that will liquidate the most amount of assets.

Ambit Team: We have discussed this in the past, however, using the position that has the largest USD value (ignoring the discount) is preferrable as it allows for the greater portion of liabilities to be repaid.

The Custodian is kept as a separate, immutable variable inside of the TokenRewardHooks instead of _asset.custodian being used. Upon Custodian migration through the CustodianMigrator, the address of the Custodian will change and TokenRewardHooks will continue to reference the old Custodian. This necessitates a TokenRewardHooks re-deploy as _custodian cannot be reset.

Consider using the Custodian directly on the _asset instead of setting _custodian.

Ambit Team: The issue was resolved in commit 310287d.

Any asset, that gets disabled will have it's balance hijacked due to a lack of access control on claim(). Function claim() calls accrue(_tokenRewards[token]), which is a private function only also called in accrue(). This function updates the current index of each epoch on the token based on the totalSupply of custodian shares at the moment.

Due to the asset not being in the _rewardTokens anymore, it does not get its index updated upon a user change.Consequently claim() is prone to a Portfolio share inflation attack that steals the whole balance of the said reward token.

This can be achieved by depositing a large amount of the asset, calling claim(), which updates the accrued for that token based on the now inflated balance of shares and then transfers an inflated portion to the user.

Put only active token access control on claim().

Ambit Team: The issue was resolved in commit 115e128.

During a liquidation, the total amount to be liquidated and the amount of supply to withdraw from the user’s portfolio to the Liquidator is calculated in function calculateLiquidation()

With those parameters the liquidation is settled in function settleLiquidation(), where the total amount being repaid is adjusted to be no more than the user’s liabilities: uint256 repayAmount = Math.min(context.totalAmount, context.liabilities);

Consequently, the amount to repay can be dramatically decreased yet the amount withdrawn from the user's supply is still the originally calculated supply. Although the the excess of the user’s liabilities is refunded, the supply was valued at the discounted price so the user has more supply withdrawn from their portfolio than necessary.

This results in an extra “liquidation fee” causing loss of funds for users.

Consider adjusting context.position by the discounted price to accurately reflect the amount being repaid. Otherwise, clearly document this behavior to users.

Ambit Team: Yes this is correct, however, it will only occurr for accounts that have liabilities less than the smallAccountThreshold (which would probably be set to somewhere between 250-500).

In the liquidate function, a liquidator is forced to liquidate from the asset with the largest notional value. This approach can result in less profit for the liquidator, especially when the position with the highest notional value involves a less liquid pool, this is because smaller pools experience more slippage during swaps.

This limitation makes positions with certain assets as their largest holding less attractive to liquidate due to reduced profits, increasing the likelihood of incurring bad debt.

Allow liquidators to choose which asset they want to liquidate.

Ambit Team: The MarketLiquidation contract does expose a second liquidate method that allows the liquidator to determine which asset they want to liquidate as opposed to the asset with the largest notional value. However, the initial implementation of the Liquidator is relying on the method that utilizes the findLargestPosition in order to favor paying off more debt in this scenario.

Proof of concept: PoC

The Ambit protocol supports the usage of a DiscountModel to decrease the interest accrued on a user’s borrow position. Once the DiscountModel is set, the interest is calculated with the function calculateDiscountedLiabilities instead of the typical interest calculation.

With the model a user can receive a discount for their entire borrow amount: Math.min(userLiability.borrowed, points * AMOUNT_PER_POINT).toUint128();

A user may be in the market for a prolonged period of time while there is no discount campaign and the marketState.borrowIndex will largely increase during that elapsed time. In order to avoid paying that interest, a user can wait to accrueLiabilities until a discount model is set.

A user can burn Ambit to immediately increase their points such that the discount matches the borrow position, although this may require a large amount of Ambit. Afterwards the user will call function accrueLiabilities where the resultant interest will be zero, and the user will avoid the interest that they would have had to pay otherwise.

Be extremely cautious with the discounts in the DiscountModel to prevent users from creating interest-free borrow positions. Furthermore, accrue liabilities for the user prior to burning Ambit.

Ambit Team: The issue was resolved in commit 02bedd8.

When a market is paused, interest no longer accrues for positions in the calculateMarketState function as the function ceases execution early if paused() is true. However, when a market is paused with the pause function, the pending interest is not accrued.

Therefore, when a market is paused, the interest accrued since the last accrueLiabilities call is effectively lost, as subsequent calls to accrueLiabilities update the marketState.lastUpdate timestamp without accruing interest.

Additionally, when a market is unpaused, the protocol does not ensure that accrueLiabilities is called prior to unpausing the market. This would result in the interest accrued since the last accrueLiabilities call needing to be paid, since we no longer enter the paused() is true

Call the accrueLiabilities function prior to calling pause/unpause on the market

Ambit Team: The issue was resolved in commit 48aeb70.

The treasuryFee is taken at a higher priority than the caller fee for liquidations. In the event where there is only enough profit to fully pay out the treasury fee, the treasury fee is paid and the callerFee is reduced. Therefore the incentive for a liquidator to expeditiously liquidate an account may be reduced and possibly insufficient in some cases.

Consider taking the callerFee as the highest priority so that there is always sufficient incentive for liquidations to occur in the system.

Ambit Team: The issue was resolved in commit ce65380.

In the setInterestRateModel function, the pending interest is updated with the accrueLiabilities function to correctly account for the outstanding interest having accrued under the previous interest rate model.

However in the setDiscountModel function the new _discountModel is set before the interest is updated with the accrueLiabilities function, therefore any pending interest is treated as if it had accrued with the new discount model configured while this is not the case.

Update the pending interest values with the accrueLiabilities function before updating the _discountModel in the setDiscountModel function.

Ambit Team: The issue was resolved in commit 1a1d62c.

Since the token reward epochs are being pushed into the array on every donation and never removed, there is a risk of DoS because users who supply for the first time will need to go through the entire list of expired epochs for each of the reward tokens present.

This will cost users who are supplying to their portfolio for the first time to pay an excessive amount of gas overhead, which may potentially exceed the block gas limit and prevent the user from supplying.

Consider not going through all past epochs when accruing for a new user and just setting the epoch index to the current one.

Ambit Team: The issue was resolved in commit 9b48119.

Paused tokens are not differentiated from when calling accrue() on them. This allows a user to accrue them even though they should not be by calling claim(). This then allows users to claim yield for a token that is currently on pause.

Furthermore, tokens paused through disableRewardToken() do not get accrued when calling accrue(). This would effectively lose users yield.

Consider having a check for if a token is paused or not in accrue(TokenReward storage tokenReward) and returning early if it is.

Also consider accruing paused tokens up until the pause point in order for all users to be able to claim them.

Ambit Team: The issue was resolved in commit c4b376a.

Each reward epoch restarts the accumulationIndex from 0. However in the getClaimableRewards function, the userReward.accumulationIndex is never reset when iterating through the list of epochs. Therefore the resulting claimable reward amount received from the getClaimableRewards function will be inaccurate.

Reset the userReward.accumulationIndex to zero upon iterating to a new epoch.

Ambit Team: The issue was resolved in commit 746b070.

In both the find and search functions, the method for validating whether a snapshot has been found or not is to check whether the supplied timestamp is exactly equal to the snapshot timestamp. However, this will rarely be the case as the provided timestamp is not truncated to satisfy the 5-minute intervals that the snapshot timestamps are stored with.

Truncate the provided timestamp so that it will be much more likely to line up with the snapshot timestamps.

Ambit Team: The issue was resolved in commit 1b4c59a.

The getTotalSupply function returns the totalAssets balance rather than the supply of the vault token.

However in the BaseCustodian.withdraw function, the shares amount is compared with the result of the getTotalSupply function. Therefore a shares amount is validated being against an underlying token amount.

In the check in the BaseCustodian on line 79 compare the amount which is a token amount against the result of the getTotalSupply function.

Otherwise change the definition of the getTotalSupply function, but be careful to update where it is used, as the validation using the getTotalSupply function in the Portfolio contract is currently correct as it assumes the getTotalSupply is an underlying token amount.

Ambit Team: The issue was resolved in commit 989c186.

The flash fee is not considered when approving the flash lender to pay back the flash loan, therefore the MarketplaceVendor cannot be used to deleverage positions unless the MarketplaceVendor is a flash fee whitelisted address.

The MarketplacePurchaser on the other hand does account for the flash fee.

Consider implementing flash fee support in the MarketplaceVendor. If the MarketplaceVendor contract is intended to always be a flash fee whitelisted address then be careful to always whitelist it.

Ambit Team: This is fine as the MarketplaceVendor is whitelisted and excluded from fees.

Because some liquidations require a swap, they could be prevented if a malicious actor front-runs the transaction and moves the pool price out of the acceptable range. The malicious actor could sandwich the transaction to undo this price manipulation so that minimal capital is lost.

Consequently, the liquidations will be prevented by an insufficient output amount revert, leaving bad debt in the protocol.

Use an aggregator and/or ensure the discount is large enough to meet slippage requirements. Furthermore, consider allowing unprofitable liquidations to occur, marking the forceful liquidation with a boolean flag if necessary.

Ambit Team: This should be fine as the Liquidator is the on-chain backup liquidation function. We have an off-chain liquidator that will be the primary source of liquidations and that uses 1inch to sell assets where applicable.

Since accrued points are practically points that don't get rewards accrued on them users who call accruePoints() but then do not directly claim in the same will lose out on rewards from those points.

Consider calling claimRewards() for users in accruePoints() .

Ambit Team: Users will only be subjected to claiming points which will internally accrue. getClaimablePoints() method will return the up-to-date points without a call to accruePoints().

Liability interest discounts do not apply to a user's position if they do not explicitly update their liabilities in the discount period even if they have had that position since before the discount period.

For example, if the protocol goes from a standard interest rate to a discount rate and finally back to a normal rate, the user will not experience the discounted rate at all if the function calculateUserLiability is not called during the discount period.

Clearly document to users that not updating their liabilities during the discount period will not apply the said discounts.

Ambit Team: The drawbacks are understood here, but unlikely to cause an issue as the primary purpose of the discount model is to allow points holders to have discounted rates.

Users are not supposed to be able to complete a withdrawal and supply in the same block. It is prevented with the use of the ensureWithdrawAvailability() function.

The issue arises due to the _lastUpdateBlocks mapping of a user for a particular token being deleted upon their whole balance being taken out. This allows a user to first withdraw and then supply the same token within the same block.

Do not delete the _lastUpdateBlocks mapping even if the user withdraws their whole balance.

Ambit Team: The check was only supposed to stop a supply followed by a withdraw in the same block, a withdraw followed by a supply should be fine.

When liquidating an account, market.getLiabilities(account) is called to calculate the user's liabilities. However, the user can have their liabilities reduced if their accrued loyalty points were to be claimed and consequently have a healthy position.

As a result, a user is liquidated when they could have a healthy position just by claiming their points, leading to loss of funds for the liquidated account.

Claim the account's points on the call to function accrueLiabilities.

Ambit Team: We want to encourage the users to interact with the protocol and keeping the manual claiming of points is a way to do that.

The discount percentage gets calculated as a fraction of the principle of the borrower's loan and then that percentage gets taken out of the total liabilities of the loan. The issue here is that this model heavily favors users who have a high liabilities/principal ratio.

This is against the interest of the protocol since those types of users likely haven't yet repaid large amounts of interest or have borrowed riskier assets.

Consider calculating the percentage with the total liabilities instead in order to favor users with lower accrued interest.

Ambit Team: The issue was resolved in commit 1406fa3.

One of the price oracles Ambit utilizes is the ratio of the reserves in a Uniswap pool. However, these reserves are highly susceptible to manipulation, as an attacker can manipulate the reserves of one of the tokens with a flashloan and exaggerate the price.

Due to the inflated worth of their collateral, an attacker's borrowing limit is increased and they are able to borrow more funds from the protocol than originally intended. This can drain the lending market and leave the protocol holding less value than stolen once the price of the collateral is restored.

Use the Uniswap TWAP for the price instead of the reserves calculation.

Ambit Team: The issue was resolved in commit c58344b.

In the liquidator contract the constant MIN_SLIPPAGE_ALLOWED is set to 0.5%. However, the constant is never used and no minimum slippage is enforced.

If the MIN_SLIPPAGE_ALLOWED is not intended to be used remove it.

Ambit Team: The issue was resolved in commit baa9a01.

The Portfolio provides the capability to perform a particular action before supplying/withdrawing and after supplying/withdrawing through the use of hooks on a particular asset. Because state changes occur between the before hook and after hook, if a hook were to expose an external call outside of the protocol's system, a user could reenter.

One potential issue that could arise is if a user could use the before hook to re-enter into function supply and circumvent the maximum supply validation because the Custodian's supply has yet to be updated.

Be extremely careful with the hooks that are supported on each asset, so that calls outside of Ambit are restricted. Furthermore, consider adding nonReentrant guards.

Ambit Team: This is accepted, however, hooks are still only an internal component used by the dev team when extending the protocol and this can be controlled.

The market’s utilization ratio is calculated using the instantaneous liabilities instead of using the liabilities stored in the snapshots: uint256 liabilities = depositorVault.getLiabilities(marketAddress);

Furthermore, the Snapshot.totalLiabilities is not used at all. Ultimately this may lead to some gaming of the utilization ratio, and the liabilities can be directly affected with a borrow and repay.

Ensure the fees for borrowing are large enough to deter utilization ratio manipulation. Furthermore, consider using the average of the snapshot liabilities in the utilization calculation.

Ambit Team: This has now been changed to use an average of the totalAssets. Additionally, the push the utilization higher, the user would have to borrow and in which case fees would be applied, and they would also have to have the assets supplied to their portfolio.

To push the utilization lower (and this decrease the rate) they would have to continually repay a loan and then borrow again and in which case the fees should provide an economic disincentive.

In times of volatility it may be possible for liquidations to drain the Liquidator contract of the liquidation fund. In most cases the liquidator should receive back the USDT that was used to initiate the liquidation, however in some cases when liquidating vault tokens it may be possible for the amount of USDT in the Liquidator contract to decrease.

Once the liquidator is drained no liquidations can occur and the protocol will accrue bad debt. Additionally, particularly large liquidations may eclipse the size of the current liquidation fund, in which case these liquidations would only partially occur and require several liquidations to finalize.

The risk is that the price of the collateral decreases further, increasing the chances of an insolvent position.

Be aware of this risk and ensure that the liquidation fund is always large enough to cover any liquidations.

Ambit Team: The Liquidator is just the on-chain backup, there will be an off-chain liquidator running also that will handle this.

When user's are charged fees, the fee amount is added to their principle and they simply do not receive that fee amount in USDT, instead it goes to the treasury.

Since the fee amount is still added to the user's principle however, the user still must pay interest on this fee amount that is taken. This may be unexpected for users and can cause confusion on the real amount of interest being charged.

Consider if this is the expected behavior, and be sure to document it for users.

Ambit Team: Yes this is expected behavior.

The pending rewards for an account are added to the userReward.accrued at the end of the getClaimableRewards function. However this is unnecessary as the pending rewards are already updated in the accrueRewards function call on line 258.

Remove the pending rewards calculation from the return statement in the getClaimableRewards function.

Ambit Team: The issue was resolved in commit 1c1580d.

Depositors only receive their rewards when borrowers choose to pay back their debt, however given that this is perpetual debt it is possible that depositors do not accrue their rightful interest until much later.

You lent funds in the vault in block 100, in block 101 User A borrows funds from the vault, in block 200 you withdraw funds from the vault, in block 210 User A finally repays their borrowed amount and pays out their interest to the vault.

However now you are not able to collect this interest given the time duration mismatch between the borrower leveraging the funds in the vault and actually paying back the accrued interest.

In other systems such as Abracadabra Money, the accrued interest is credited as it is generated and the system does not wait for users to repay their debt to provide yield.

Consider the drawbacks of rewarding depositors when a borrower repays their debt. In a future iteration it may be useful to consider a different method for distributing fees so that lenders are fairly compensated.

Ambit Team: The issue was resolved in commit 1c1580d.

The word “accrued” is misspelled as “accurred” on line 258.

Correct it to “accrued”.

Ambit Team: The issue was resolved in commit 929534e.

When the totalShares is 0, the accumulate function returns a 0 amount to be distributed, therefore donations will not be distributed when there is 0 shares for the _asset.

The TokenRewardHooks contract is Sweepable, so these funds may be rescued by the treasury, but consider if this is the expected behavior. If not, consider implementing a method for donators to reclaim undistributed rewards. Or for an additional incentive for suppliers to acquire shares when the supply is 0.

Ambit Team: Yes this is fine, it’s unlikely that donations will be occuring without any assets being supplied.

Accounts that are blacklisted for USDT cannot be liquidated when the refundAmount is nonzero as the settleLiquidation function would attempt to transfer USDT to the blacklisted account. On BSC there is no blacklist functionality for USDT, however on other chains USDT does have a blacklist feature, so care should be taken when deploying to new chains.

Carefully consider new chain deployments as certain features may be incompatible or exploitable on new chains.

Ambit Team: When we do deploy to these chains we will look at the option to actually just keep the funds in the treasury if the account is blacklisted.

There is a typo in the variable ellapsed as it should be named elapsed.

Rename the ellapsed variable to elapsed.

Ambit Team: The issue was resolved in commit 929534e.

In the setBorrowLimit function, there is no safety check that the provided amount is within the max for a basis point value if absoluteOrRelative is AbsoluteOrRelative.RELATIVE.

Consider adding a safety check that the amount is within the expected range for basis point values if the absoluteOrRelative value is AbsoluteOrRelative.RELATIVE.

Ambit Team: The issue was resolved in commit 382b0ad.

In the afterSupply and afterWithdraw functions the accrue method is invoked. However the accrue method will have already been called previously as a part of the beforeSupply and beforeWithdraw functions.

Remove the redundant calls to accrue in the afterSupply and afterWithdraw hooks.

Ambit Team: The issue was resolved in commit 60a421b.

In the enableRewardsToken function the tokenReward.index is assigned to 0 if the tokenReward.index is 0 and otherwise it is assigned to the tokenReward.index. This is pointless and therefore the line can be removed.

Remove the superfluous tokenReward.index assignment on line 136.

Ambit Team: This was removed with the update to token rewards referenced in a few other issues.

There is no migration logic for the yield bearing custodian.

Consider if this is the expected behavior, implement migration logic if it should be migratable.

Ambit Team: The issue was resolved in commit eeadec7.

There is no implementation for the previewDonationFee, therefore there will be no donation fee or fee receiver.

Be sure this is the desired behavior for the YieldBearingCustodian donations.

Ambit Team: Yes this is fine, only the DepositorVault has a donation fee.

When the depositorVault is assigned in the registry the depositor vault key is recomputed by hashing the “ambit.depositorVault” string. However it would be a better practice to access and use the already computed keys from the AddressRegistryExtensions file. This way potential typos can be avoided when they would cause critical issues.

Rather than manually hashing the key for the depositor vault, use the pre-computed AMBIT_DEPOSITOR_VAULT key from the AddressRegistryExtensions contract.

Ambit Team: The issue was resolved in commit b8239f5.

The DepositorVaultMigrator contract inherits from both the AdminAccessControl and AuthorizedAccessControl contracts.

However the AuthorizedAccessControl also inherits from the AdminAccessControl contract. Therefore it is unnecessary for the DepositorVaultMigrator contract to directly inherit from the AdminAccessControl contract.

Remove the direct inheritance from the AdminAccessControl in the DepositorVaultMigrator contract.

Ambit Team: The issue was resolved in commit 77197c8.

The getExchangeRate ought to return an exchange rate of _scalar in the case where the totalShares is 0 as this will be the exchange rate for any deposit when the totalShares are 0.

Consider updating the getExchangeRate implementation such that it returns _scalar when the totalShares is 0.

Ambit Team: The issue was resolved in commit 6d498c3.

There is no need to truncate the snapshot.timestamp a second time as the timestamp has already been truncated on line 38.

Remove the truncate function call on line 41.

Ambit Team: The issue was resolved in commit 6d498c3.

Trusted liquidators have the capability to liquidate an arbitrary amount and from any asset in a user's portfolio. This flexibility allows a liquidator to intentionally liquidate specific assets and amounts in a strategic order to maximize their profit.

If a liquidator performs two liquidations— the first being as much as possible while keeping the position unhealthy, and the second being the full 50% of the user's largest position—they can liquidate more than if they had only done the 50% liquidation in the first attempt.

It is recommended to clearly document that there is no hard cap for how much a user can be liquidated.

Ambit Team: Liquidation will be restricted to trusted actors so this shouldn't present a problem initially. If its decided to open this up then we will revisit.

In the find function on line 73, in the event that the queried timestamp is in the middle of the latest and oldest timestamps, the find function always returns true for the found result.

However a snapshot match might not have been found in the search function, and instead the first snapshot that is larger will be returned. This may not fit the consumer’s idea of the found boolean from the find function.

Consider if the search function should determine whether a snapshot was found or not depending on if the match case was hit.

Ambit Team: The found parameter is supposed to represent that the requested timestamp was prior to any data that currently exists in the history and therefore the value could not be found, however, it returns the first available value in this case (which should be after the timestamp requested).

• 1e8 AMBT = 10 PTS
• 1 PTS = 1e10 Wei Discount Amount (assuming 18 decimal precision of underlying vault token)
• 1e8 AMBT = 1e11 Wei Discount Amount
• USDT = 1e18 Wei
• 1e15 AMBT = 10,000,000 AMBT = 1e18 Wei Discount Amount
• To obtain 1 USDT discount a user may require 10,000,000 AMBT which is 10% of its supply.

Document to users this behavior and/or increase the discount.

Ambit Team: This issue was resolved in commit 9fb99ed

In the buy function, the amount is first validated to be less than the maxAmount and the adjusted maxAmount that is subsequently used to flash loan is the minimum of the amount and the maxFlashLoan available in the FlashLender.

However when the maxFlashLoan is less than the amount the flash loan will always fail with the validation later on in the onFlashLoan function on line 130. Therefore this case is not validated early enough and needlessly allowed by the Math.min operation on line 98.

Validate that the amount is less than the maxFlashLoan directly in the buy function to avoid unnecessary logic.

Ambit Team: This issue was resolved in commit 5e78aa3

Calls to safeTransferFrom should generally occur before any state updates in a function to avoid yielding an invalid state where updates have occurred before the tokens have been received.

Move uses of safeTransferFrom to the beginning of the repay and deposit functions in the DepositorVault.

Ambit Team: This issue was resolved in commit 6652b39

block.timestamp is used as an expiration timestamp for UniswapV2 swaps, practically the same as passing no timestamp. This presents an issue as the swap will go through no matter if the transaction stays in the mempool for a prolonged period.

Consider using a timestamp passed from the caller.

Ambit Team: This issue was resolved in commit 9b9ddda

In the calculateDiscountAmount function, if the vault asset’s decimals is less than 8 decimals the user can lose points due to precision loss when the normalize function is called:

uint256 points = loyalty.getPoints(account).normalize(8, decimals);

To minimize the loss of points perform multiplication before division by normalizing points * AMOUNT_PER_POINT instead of just points.

Ambit Team: This issue was resolved in commit a414931

The following return (rate - DISCOUNT_RATE.percentOf(rate)).toUint128(); is incorrect as percentOf() returns uint256. Instead of only DISCOUNT_RATE.percentOf(rate) being cast, the whole expression is cast.

Consider re-implementing the expression like so:

return rate - (DISCOUNT_RATE.percentOf(rate).toUint128());

Ambit Team: This issue was resolved in commit 8df6db8

emit Donate(msg.sender, amount - feeAmount, feeAmount, feeReceiver);

The event above logs the donation amount as amount - feeAmount even though the fees also get logged by the event.

Consider changing amount - feeAmount to amount.

Ambit Team: This issue was resolved in commit 5126b26

LinearDistributedYieldVault distributes interest even if there are no shares/depositors in it. This will allow the first share depositor to get all the leftover yield.

Consider not distributing yield if there are no shares in the vault.

Ambit Team: This is fine as it should be rare that we yield without any shares as in theory if there's nothing in the vault then there is nothing to borrow to generate yield anway. Additionally if this did occur, it should encourage users to deposit.

When migrating to a new DepositorVault instance the first snapshot in the observer gets set as the latest params instead of copying the last few snapshots.

Consider copying the last n snapshots into the new DepositorVault.

Ambit Team: This is fine given;

• the frequency that we migrate the vault will be very low
• there's only a small window (10-15 mins) where the rate could be artificially forced low

All positions are denominated in USD with 8 decimals of precision. A very small loan in a token with less precision than the USD precision will get truncated to 0 when normalizing the decimals in uint256 total = totals[j - 1].normalize(_registry.getDepositorVault().getDecimals());

This will then offset all following calculations in liquidateInternal() and will not allow for the position to be liquidated, leaving the protocol with bad debt it cannot remove.

Consider introducing a minimum deposit amount.

Ambit Team: This would be a 0 total as you’ve identified. However, the user wouldn't even be able to borrow against that position in the first place as the borrow limit is also normalized to the decimal precision of the base asset. It's a different story if the price of the asset was a lot higher and then crashed, but that's an inherent risk with an oracle based lending protocol.

uint256 elapsed = block.timestamp - marketState.lastUpdate; gets calculated before timestamp validity gets checked.

Consider calculating elapsed after the check.

Ambit Team: This issue was resolved in commit f698173

Since Loyalty and TokenRewardHooks rely on hooks to accrue points and update indexes each time there is a change in the user's portfolio, freezing the hooks of a token will cause all logic in one of those two systems to experience accounting issues.

Consider implementing contract freezing logic for the two systems to freeze all their logic in regards to the particular token when its hooks get removed.

Ambit Team: This issue was resolved in commit cfd4e20