Sentiment V2

Proof of concept: PoC

When assets are withdrawn from SuperPool, the redeem function in the base pool is invoked. This function requires the share amount as an input. However, during this call, the asset amount is provided instead, leading to significant accounting issues and potential loss of funds.

To address this issue, it is recommended to convert the user-provided asset amount to the corresponding share amount before proceeding with the redemption process.

Sentiment Team: The issue was resolved in PR#222.

Proof of concept: PoC

Users deposit collateral and borrow assets using the PositionManager contract, and every action that changes a Position’s balance requires a health check via RiskEngine and RiskModule.

The health check is done by comparing total debt of a position and total asset of a position. However, this health check is inaccurate when a borrowed asset and the collateral asset are the same. The RiskEngine accounts newly borrowed assets as user provided collateral, which causes the check to be incorrect.

A pool owner can

• Set the ltv to 1 for the borrow asset.
• Add the borrow asset as collateral asset to his position with addToken
• Borrow all assets from the pool without adding any collateral.

Resulting in all funds to be frozen for regular depositors.

Do not allow borrow asset and the collateral asset to be the same.

Sentiment Team: Resolved.

Proof of concept: PoC

Users can borrow from the Pool only by using the PositionManager. Although users must deposit enough collateral to pass the health check after borrowing, the Pool does not check if the poolId has enough liquidity to support the amount borrowed.

Any amount borrowed that exceeds the poolId liquidity, will effectively steal this liquidity from other pools, and total borrows will be greater than total assets.

There are multiple impacts with this issue:

• pool redeems are DoS'ed when calculating:

uint256 assetsInPool = pool.totalAssets.assets - pool.totalBorrows.assets

• lenders won't be able to redeem from other pools, as there is not enough assets in balance
• SuperPool maxWithdraw reverts as getLiquidityOf calculation will underflow

Prevent positions from borrowing more assets than the liquidity of the poolId.

Sentiment Team: The issue was resolved in PR#243.

Proof of concept: PoC

SuperPool vault shares should have the same decimals as the underlying ASSET. The decimal value is set in the constructor. However, this is not the case, as depositing 1e18 assets will give you 1e36 shares.

The issue relies on _convertToShares when the first user deposits, where lastTotalAssets and totalSupply are 0, but it will multiply by 10 ** DECIMALS:

shares = assets.mulDiv(totalSupply() + 10 ** DECIMALS, lastTotalAssets + 1, rounding);

Although users will be able to redeem the shares for the correct amount of tokens, there is a discrepancy between the decimals() of the vault and the minted share units. Users will be able to use this vault token as collateral to borrow assets against. When calculating the asset value of the vault token in RiskModule, the value returned will be 1e18 times greater than expected.

Therefore, users will be able to drain pools by borrowing all assets, as the collateral value is basically infinite: vaultTokens(1e36) * priceInEth(1e18) / decimals(1e18) = 1e36 ether value

Implement the following:

• shares = assets.mulDiv(totalSupply() + 10 ** DECIMALS, lastTotalAssets + 1, rounding);
• shares = assets.mulDiv(totalSupply() + 1, lastTotalAssets + 1, rounding);
• assets = shares.mulDiv(lastTotalAssets + 1, totalSupply() + 10 ** DECIMALS, rounding);
• assets = shares.mulDiv(lastTotalAssets + 1, totalSupply() + 1, rounding);

Sentiment Team: The issue was resolved in PR#240.

Proof of concept: PoC

The _getMinReqAssetValue function in the RiskModule contract determines the minimum required asset value for a position to be considered healthy. This function is called by the isPositionHealthy function, which performs a health check after every action or series of actions taken by users to ensure their position remains healthy.

The issue arises because the _getMinReqAssetValue function relies on the length of the position's positionAssets array for the inner loop when calculating the required asset value for a position to be healthy. However, a position does not need to have any assets added to its positionAssets list in order to perform a borrow.

As a result, a user could perform a borrow with no funds, and the health check performed at the end would still pass. This occurs because _getMinReqAssetValue would return zero, given that the position's positionAssets is empty, despite the position having open debt from the borrow. Consequently, a user could borrow all funds from a pool and transfer them to personal accounts, effectively draining the pool.

Update the _getMinReqAssetValue function to revert if the resulting minReqAssetValue is zero. This function is only called when debt exceeds zero, so minReqAssetValue should almost always be greater than zero.

One exception is when a position's value falls to zero, causing insolvency. In such cases, this fix could block the liquidation of bad debt, so an admin function should be implemented to handle these scenarios.

Sentiment Team: The issue was resolved in PR#271.

Proof of concept: PoC

SuperPool owners can adjust the fee anytime instantly and no boundaries for the fee are set. This enables a honeypot attack:

• Attacker creates a SuperPool with a 1% fee
• Users deposit into the pool
• The attacker sets the fee way above 100%
• A few seconds pass and the pending interest for the attacker is >= all assets in the contract
• Attacker withdraws all funds of the SuperPool

Implement min/max values and a timelock for critical parameter updates like fees.

Sentiment Team: The issue was resolved in PR#225.

Proof of concept: PoC

• Anyone can create a BasePool with an arbitrary contract as RateModel
• Anyone can create a SuperPool
• The owner of a SuperPool can add a new BasePool to a SuperPool and reallocate all funds to it any

time

These conditions allow the following attack:

• SuperPool owner creates a BasePool with a malicious RateModel and mints some shares
• SuperPool owner adds the BasePool to the queue of the SuperPool
• SuperPool owner reallocates all funds to the malicious BasePool
• The malicious actor calls accrue on the BasePool and the malicious RateModel returns that a lot of

tokens in interest were accrued

• The malicious actor withdraws all funds in the BasePool with the few shares minted in the

beginning and the users of the SuperPool lose everything

Do not allow arbitrary addresses as RateModel.

Sentiment Team: The issue was resolved in PR#264.

• The interestFee and originationFee are fees that go to the protocol.
• Anyone can create a new BasePool and set the interestFee and originationFee while doing so.

Therefore the protocol fees are not enforced and act like donations instead. As it makes no economic sense for the BasePool owner to set these fees above 0 the protocol will probably lose a lot of money.

Enforce the protocol fees.

Sentiment Team: The issue was resolved in PR#224.

Proof of concept: PoC

In the reallocate function it is not checked if the given pools to deposit to are part of the deposit/withdraw queue. Therefore malicious SuperPool owners can deposit funds into a pool that is not in the queue, which will freeze funds for the users of the SuperPool. The owner could then spread on social media that users need to pay X amount of funds to unfreeze it (this could be even done with a contract). When the users pay the amount, the owner can unfreeze the funds by adding the new pool to the queue or reallocating the funds back to the original pool.

Add a check to see if the pools are part of the deposit/withdraw queue.

Sentiment Team: The issue was resolved in PR#231.

Proof of concept: PoC

• The owner of a BasePool can set the LTV of a token to zero.
• The isPositionHealthy function will revert if a position holds a token in the asset list and this token

has an LTV of zero.

Therefore if the owner updates the LTV of a token to zero and any position holds this token any interaction with it through the PositionManager will revert.

This has multiple bad consequences:

• Borrowers debt increases but they are not able to repay
• Borrowers can not be liquidated
• Borrowers are not able to get their collateral back
• The owner of a base pool can create unliquidatable positions

Unliquidatable positions:

• Owner sets the LTV of a token above 0
• The owner creates a position adds this token as collateral, borrows funds, and does something

risky with them

• The owner sets the LTV for this token to 0
• Now the position is not liquidatable till the owner sets the LTV back to a value above 0

Do not allow to set the LTV of a token to zero.

Sentiment Team: The issue was resolved in PR#237.

Proof of concept: PoC

SuperPool contract has deposit and withdrawal queues. Order of these queues are important since deposits and withdrawals are done based on the order, which is supposed to be arranged by the owner.

However, due to incorrect implementation of the _reorderQueue function, the owner can not change queue orders. This function takes a new order as an indexes array, and is supposed to rearrange the order based on indexes. But it only copies the previous order as is with the newQueue[i] = queue[i] line.

Use the inputted indexes array to determine new order. Change newQueue[i] = queue[i] to newQueue[i] = queue[indexes[i]].

Sentiment Team: The issue was resolved in PR#220.

• The _removePool function reverts if the SuperPool still owns assets in the given pool.
• Anyone can deposit into a BasePool and set any SuperPool as the receiver of the assets.

This enables a malicious actor the possibility to front run a _removePool call and deposit one wei of assets into the SuperPool to DoS the call.

Implement a function that reallocates and removes the pool in one transaction.

Sentiment Team: The issue was resolved in PR#266.

Proof of concept: PoC

The last three actions in the withdrawal flow of the SuperPool are:

• Burning the share tokens
• Transferring the funds to the user
• Reducing the lastTotalAssets value

Therefore if the attacker reenters on receiving the tokens the total amount of shares is already reduced, but the total amount of assets is not.

The first action in withdraw/deposit is accruing interest calculated based on the saved lastTotalAssets value and the current total amount. Therefore the given difference on reentering (as lastTotalAssets is not reduced yet) is seen as interest and the owner of the pool receives fees on this interest.

This enables the following attack path:

• SuperPool owner deposits tokens into the own pool
• SuperPool owner withdraws the tokens and reenters on receiving them over and over again
• Every time the difference is seen as interest and the owner receives fees
• Owner withdraws the gained fees
• The owner repeats this process over and over again till the SuperPool is drained completely

• safeTransfer should be the last action in this flow.
• Use a reentrancy guard on critical functions.

Sentiment Team: The issue was resolved in PR#244.

Proof of concept: PoC

The LinearRateModel calculates the totalAssets amount by summing up the totalBorrows and the idleAssetAmt. But the idleAssetAmt already is the totalAssets amount (pool.totalAssets.assets from the BasePool). This leads to an incorrect calculation of the utilization ratio.

For example: Unborrowed Amount = 500 Borrowed Amount = 500 Therefore util ratio = 50% Calculation in the LinearRateModel: idleAssetAmt = pool.totalAssets.assets = 1000 totalAssets = totalBorrows + idleAssetAmt = 500 + 1000 = 1500 util = totalBorrows / totalAssets = 500 / 1500 = 33.33%

Therefore the calculated utilization ratio is 33.33% when 50% of the funds are borrowed. As the utilization ratio is smaller than it should be the lender receives less interest than they should.

Do not calculate the totalAssets amount as the idleAssetAmt already is the totalAssets amount.

Sentiment Team: The issue was resolved in PR#227.

Proof of concept: PoC

Users can pay their Position’s whole debt by passing type(uint256).max value as the repayment amount. In that case, PositionManager contract will calculate Position’s total debt and make a call to the underlying pool for the payment.

However, the getBorrowsOf function uses convertToAssets, which rounds down by default, and this causes amount to be paid to round down. This amount is later used in the repay function. repay in the Pool contract also rounds down the borrowShares amount to burn, which is done to ensure excess debt isn't pushed to other users.

As a result of rounding down twice during the repay flow, the Position will always have 1 borrowShare left even though the user tries to pay whole amount unless the amount is exactly the multiple of asset:share ratio.

This would cause repayment to fail due to MIN_DEBT requirement. Also, even if the Position has other debts that cover MIN_DEBT, the repaid debt pool can not be removed from debtPools array from the Position contract due to remaining 1 share.

Firstly, getBorrowsOf function should round up. Then, ensure that all borrowShares are burned when all debt is paid.

Sentiment Team: The issue was resolved in PR#245.

The SuperPool owner has the capability to reorder deposit and withdrawal queues. However, there is a lack of duplicate entry check in the _reorderQueue function, which could potentially enable a malicious owner to freeze funds.

For instance:

• withdrawQueue = [1, 2, 3]
• A malicious owner reorders with indices [0, 0, 0]
• Resulting withdrawQueue = [1, 1, 1]

As a result, the funds deposited in pools 2 and 3 become unwithdrawable.

It is recommended to implement a duplicate check within the function to prevent this issue from occurring. Verify the indexes param in the reorder queue functions contain all pool ids in the queue array.

Sentiment Team: The issue was resolved in PR#232.

Proof of concept: PoC

The given maxLtv value provided by the protocol as a deployment parameter is 1e18 which equals a 1:1 ratio LTV. This means users can borrow assets for $100 by providing $100 collateral if the LTV for the given collateral token is set to 1e18. Therefore only a small price change not updated in the oracle because of deviation, or not yet updated as the attacker front runs the oracle update can be abused to make risk-free profit and put the pool into bad debt.

Here is a possible attack scenario:

• The pool owner adds a new asset to the BasePool with a 1e18 LTV (1:1 ratio)
• The price of this asset is currently outdated as chainlinks deviation threshold is not reached (for

example 0.5% deviation and 0.4% change)

• The attacker takes a flash loan of the given asset and borrow all funds with it
• The attacker sells the funds from the BasePool for a 0.4% - trading fees profit and pays back the

flashloan

• The attacker made a large profit depending on the size of the pool and the pool will be in bad debt

as soon as the oracle price is updated

Use a lower maxLtv value.

Sentiment Team: The issue was resolved in PR#281.

Every deposit call could fail if the BasePool is paused, or if the cap in the BasePool is reached. As this call is not wrapped into a try-catch block, the transaction will revert and therefore the user is not able to deposit into the other pools of the queue. For example, if the first BasePool in the queue is paused the whole SuperPool deposit function is not usable.

Wrap the deposit call into a try-catch block as it is done with withdraws.

Sentiment Team: The issue was resolved in PR#234.

The _withdrawFromPools function uses the getAssetsOf function of the given BasePool to calculate the maximum that can be withdrawn. But the getAssetsOf function returns the amount that the SuperPool owns in the BasePool, not the amount that the SuperPool can withdraw right now.

This can lead to reverts and using the queue in a suboptimal way:

• The SuperPool has 100 tokens in the BasePool
• The user tries to withdraw 15 tokens
• The util ratio of the BasePool is 95% (Only 10 tokens can be withdrawn right now as the rest is

borrowed)

• Therefore the SuperPool could withdraw 10 tokens from this pool and 5 tokens from the next one
• Instead, it tries to withdraw 15 tokens from this pool, the call fails and it will try to withdraw 15 from

the next one

• This reorders the queue in a suboptimal way

Reduce the borrowed funds from the assetsInPool, or set the assetsInPool to the balance of the BasePool if it's smaller. Do not withdraw more than available liquidity.

Sentiment Team: The issue was resolved in PR#235.

The _supplyToPools function loops through all BasePools in the queue and supplies funds to them as long as the cap is not reached. But this function does not revert if all caps were reached and it was impossible to supply funds to any pool. Therefore lenders are still able to deposit funds into a full SuperPool and mint shares which is very capital inefficient and will reduce the yield per share of all lenders.

Example:

• X lenders deposit a sum of Y USDC into pools with 5% APY over the SuperPool and reach the cap
• These lenders now receive 5% APY on their deposits
• More lenders deposit into the SuperPool which is already full
• The new lender's funds are not put to work but they still receive shares of the SuperPool and

therefore a share of the yield from the BasePools

• Every lender now receives less than 5% APY on their deposits

Revert at the end of the _supplyToPools function.

Sentiment Team: Acknowledged.

Proof of concept: PoC

The initializePool function allows users to initialize a new pool, which users can then deposit the pool's assets into as well as access other functionalities. The initializePool function enforces a check to ensure that an already initialized pool can’t be reinitialized, as this would reset the pool's totalAssets and totalBorrows back to zero, wiping out all previous users' deposits and debt.

This check is done by ensuring that ownerOf[poolId] is zero, which implies the newly created pool does not currently exist. The problem is that the initializePool function allows the owner address to be set to zero. Therefore, a pool could be created with the ownerOf[poolId] as address zero, and since this check is used to ensure a pool is not being reinitialized, this will be bypassed in this case, and the pool can be reinitialized, resetting all values.

Since the zero address is used as a check to ensure a pool ID does not already exist in the initializePool function, restrict users from being able to set the owner parameter as the zero address to avoid the problem above.

Sentiment Team: The issue was resolved in PR#238.

Proof of concept: PoC

Users can borrow funds from pools using the PositionManager contract, as long as the position remains healthy, verified by riskEngine.isPositionHealthy(position).

The isPositionHealthy function has a flaw, as it has a condition where it can revert: if (totalDebtValue != 0 && totalDebtValue < MIN_DEBT) revert RiskModule_DebtTooLow(position, totalDebtValue);

The liquidate function uses isPositionHealthy to avoid liquidating healthy positions, but if the position is not healthy and the totalDebtValue is less than MIN_DEBT, the liquidation fails. Although isPositionHealthy is checked when a position borrows assets, the eth value of the debt can decrease below MIN_DEBT.

Consider removing the MIN_DEBT check from RiskModule and add it only to the borrow and repay functions in PositionManager

Sentiment Team: The issue was resolved in PR#270.

The feeRecipient is initially set during the deployment of a SuperPool and can also be changed using the setFeeRecipient function in the SuperPool contract. This address receives fees when interest is accrued, which are minted as SuperPool shares to the feeRecipient.

The issue is that there is no restriction on the address to which the feeRecipient can be set. As a result, a SuperPool owner could inadvertently or maliciously set the feeRecipient to the zero address. This will cause the accrue function, which is called in most operations, to revert, as the SuperPool _mint function will revert when the recipient address is zero.

To prevent this issue, add a validation check in the deploySuperPool and setFeeRecipient functions to ensure that the feeRecipient address is not set to the zero address when the fee is greater than zero.

Sentiment Team: The issue was resolved in PR#246.

Proof of concept: PoC

All pools exist within the singleton pool contract, with one singleton contract per SuperPool factory. Developer comments indicate that SuperPools should only be deployed through the factory contract to ensure they all point to the same singleton pool implementation.

However, the constructor of the SuperPool lacks a check to confirm it is being called by the factory. This opens the possibility for users to deploy a SuperPool that points to a malicious base pool. With multiple SuperPools containing the same asset, distinguishing between malicious SuperPools and regular ones becomes challenging for ordinary users.

Implement an onlyFactory check to restrict SuperPool deployment to the factory only, preventing unauthorized users from deploying potentially harmful SuperPools.

Sentiment Team: The issue was resolved in PR#268.

Proof of concept: PoC

Some ERC-20 tokens return a boolean instead of reverting therefore using transferFrom will not revert when the transfer fails.

This can be abused in the liquidate function as the flow looks like the following:

• The liquidator repays the debt of the borrower:
• The funds are transferred from the liquidator to the position (if the transfer fails with such a token

nothing happens here)

• The debt of the borrower is reduced in the pool
• The liquidator seizes funds from the position as reward

Therefore if the transferFrom call failed without reverting the debt of the borrower is reduced but the funds in the pool stayed the same and the liquidator receives rewards from the position for free.

Use safeTransferFrom instead of transferFrom.

Sentiment Team: The issue was resolved in PR#219.

The exec function of the Position contract can transfer native ether, as this might be needed to interact with some protocols (for example to pay gas for a 2-step flow).

But it is not possible to use this feature as ether can not be deposited into the Position contract:

• The deposit function can not be used to deposit native ether
• The exec function is not payable
• And there is no receive or fallback function in the Position contract

Implement the possibility to deposit native ether and/or make the exec functions payable and forward the msg.value from the PositionManager to the Position contract.

Sentiment Team: The issue was resolved in PR#247.

The superPoolCap variable is initialized and can be updated, but is never checked in the deposit flows (only in the maxDeposit view function).

Check if the superPoolCap is reached.

Sentiment Team: The issue was resolved in PR#228.

The addToken function is paused when the PositionManager is paused. This prevents borrowers from adding new tokens as collateral to their position, which could result in the borrowers not being able to keep their position healthy.

Here is an example of such a scenario:

• A borrower opens a position with a collateral token (for example a BasePool or SuperPool share

token) and borrows funds

• Something bad happens in the system and the PositionManager as well as the pool of the

collateral token is paused

• The collateral of the borrower losses value
• As the pool is paused the borrower is not able to get more tokens and increase the collateral of the

position

• Also as the addToken function is paused the borrower is not able to add a new collateral token to

the position The same could happen with another token that is for any reason not available at the given moment.

Remove the whenNotPaused modifier from the addToken function.

Sentiment Team: The issue was resolved in PR#267.

Module addresses are fetched from the Registry contract and these addresses are stored with key to address mappings. Address keys are constant variables in contracts and they are determined using keccak hashes.

SENTIMENT_POSITION_BEACON_KEY in the PositionManager contract is stated as “0xc77ea3242ed8f193508dbbe062eaeef25819b43b511cbe2fc5bd5de7e23b9990”. However, the correct hash result of keccak(SENTIMENT_POSITION_BEACON_KEY) is “0x6e7384c78b0e09fb848f35d00a7b14fc1ad10ae9b10117368146c0e09b6f2fa2”.

If the Registry contract owner uses the correct key while setting addresses, positionBeacon address in the PositionManager contract will be retrieved incorrectly from the Registry contract.

Use the correct hash for constant keys.

Sentiment Team: The issue was resolved in PR#226.

In the current liquidate flow the liquidator has to first repay debt from the own pocket and receive funds from the liquidated borrower's position after that. If the unhealthy position is very big, fewer users (or no one) might be able to repay it from their own wallet.

Reversing the order of this flow (seize before repay) could enable more users to have enough funds to repay the debt. This leads to more liquidations on time and therefore decreases the likelihood of bad debt.

Reverse the order of the repay and seize loop in the liquidate function.

Sentiment Team: The issue was resolved in PR#248.

The same heartbeat (one hour) is assumed for all chainlink price feeds, but there are assets with different heartbeats for example 24 hours this will result in all operations with a 24-hour heartbeat asset to only work 1 hour a day and DoS the rest of the time.

Implement the possibility to set the heartbeat for each price feed individually.

Sentiment Team: The issue was resolved in PR#239.

At the moment the system does not handle bad debt, the liquidator always has to repay the full loan. If the loan to repay is higher than the assets in the Position it makes no economic sense to call liquidate as the liquidator would lose money. This leads to no one calling liquidate if bad debt occurs and the bad debt probably increases even further.

The missing opportunity to take bad debt would lead to major problems in the system when a black swan event occurs.

Handle bad debt by repaying the maximum amount possible and either reducing the amount owned by the lenders or increasing the debt of the borrowers.

Sentiment Team: The issue was resolved in PR#272.

SuperPool is a contract that is compatible with ERC4626. It includes multiple preview functions that, in the end, call internal functions _convertToShares or _convertToAssets.

These internal functions utilize the lastTotalAssets variable in their calculations. However, lastTotalAssets does not represent the most updated asset amount, as it does not account for accrued interest since the last update. Therefore, the results of the preview functions are not accurate.

Some more examples of non-compliance include:

• previewDeposit does not simulate accrue, so deposit might mint less shares than previewed.
• previewMint does not simulate accrue, so mint might consume more assets than previewed.
• previewRedeem does not simulate accrue, so redeem might withdraw less assets than previewed.
• previewWithdraw does not simulate accrue, so withdraw might burn more shares than previewed.
• maxDeposit/maxMint do not correctly return the amount that can be deposited, as the cap can be

bypassed.

• maxDeposit/maxMint do not return 0 if pools are paused for deposits
• maxWithdraw/maxRedeem return more assets than the real amount available, as

pool.getLiquidityOf adds interest accrued.

• deposit should revert if all of assets cannot be deposited (due to poolCap limit)

To ensure accurate preview calculations, it is suggested to call the simulateAccrue function and utilize the returned newTotalAssets value. This will provide a more precise calculation of assets including accrued interest. Furthermore, consider making the Superpool EIP-4626 compliant.

Sentiment Team: The issue was resolved in PR#240.

The origination fee (borrowing fee) rounds down (in favor of the borrower) and no minimum borrowing amount is enforced. Therefore borrowers can avoid paying borrowing fees by borrowing dust amounts multiple times.

For example: originationFee = 0.01e18 (1%) Amount to borrow = 99 fee = amt _ originationFee / 1e18 99 _ 0.01e18 = 0.99e18 fee = 0.99e18 / 1e18 = 0.99 = 0

This will likely lead to a loss (because of gas fees) for the borrower on most tokens (1e18 precision) but it could be profitable with low-precision tokens.

Implement a minimum borrow amount or round up (against the borrower).

Sentiment Team: The issue was resolved in PR#269.

The PositionManager contract contains two mappings, isKnownAddress and isKnownFunc, which define the universe of the protocol. isKnownAddress specifies recognized addresses that a Position can interact with.

The transfer function verifies whether the token is known as expected. However, the deposit function lacks this validation. Borrowers can deposit any asset to their position as collateral, causing these tokens to become locked since users are unable to transfer them afterwards. An analogous issue is present in the addToken function.

Ensure that the address is verified in these functions.

Sentiment Team: The issue was resolved in PR#250.

Some updates in the protocol require a 24-hour timelock period. These updates are requested initially and then accepted or rejected after the timelock period has elapsed.

However, the requester, accepter, and rejecter are all the same person. A malicious owner could request an update days or weeks before it is actually needed and simply wait for the opportune moment to accept it, rendering the timelock feature ineffective.

Consider implementing a deadline, such as 12 or 24 hours, for accepting a request after the timelock period has elapsed. Do not allow a pending request to be accepted after this deadline.

Sentiment Team: The issue was resolved in PR#251.

Some functions such as borrow, addToken, and removeToken in the PositionManager contract have the whenNotPaused modifier. The exec function should also have this modifier to prevent any unwanted actions from being executed when paused.

Include the whenNotPaused modifier in the exec function. Additionally, reassess other functions like transfer to determine if they should be permitted when paused.

Sentiment Team: The issue was resolved in PR#267.

Proof of concept: PoC

The BasePool uses internal variables to keep track of the pool balance as well as the user's balances and does not use the actual balance of the contract. Therefore rebalance and fee on transfer tokens will not work properly in the system.

Here is an example of how a fee on transfer token would act in the system:

• User1 deposits 1000 tokens to the pool
• User2 deposits 1000 tokens to the pool
• User1 withdraws 1000 tokens from the pool
• User2 tries to withdraw 1000 tokens from the pool, but the call will revert as the pools balance is

lower than 1000 tokens because of the fee on every transfer

Update the system to support rebasing and fee on transfer tokens, or do not allow them by not setting oracles for these tokens and explicitly state these tokens are not supported.

Sentiment Team: The issue was resolved in PR#265.

The Pool and PositionManager contracts utilize the Registry contract to retrieve crucial module addresses, and both contracts feature an updateFromRegistry function to update these addresses. In addition, both contracts are equipped with a setRegistry function that enables the owner to modify the Registry contract address.

It is advisable to invoke the updateFromRegistry function when setting a new Registry address in order to avoid potential mismatches. Failing to do so may result in the utilization of outdated module addresses until the updateFromRegistry function is manually called.

Always call the updateFromRegistry function when implementing a new Registry address.

Sentiment Team: The issue was resolved in PR#252.

Proof of concept: PoC

The liquidate function in the PositionManager contract allows anyone to liquidate an unhealthy position. Specifically, a user can select the amount and assets of debt to repay from the position and the amount and assets to seize from the position in return. The requirements being that the position must initially be unhealthy and end up being healthy after the liquidation, while also enforcing a maximum limit on the asset value that can be seized by the liquidator.

The issue is that, with the current implementation, the liquidator can seize assets not included in the position’s positionAssets list, as long as they are known assets. This should not be allowed, as the health check performed on the position only considers the assets in the position’s positionAssets list. Consequently, a user could unfairly lose assets that they did not intend to risk in a position by leaving them out of the asset list.

Modify the liquidate function to ensure that liquidators can only seize assets from a position's positionAssets list.

Sentiment Team: The issue was resolved in PR#253.

The acceptRateModelUpdate function allows the pool owner to change the pool's rate model to the pending one after the timelock duration (one day). This rate model is used in the simulateAccrue function to calculate the interest accrued in the pool for the duration since it was last called (pool.lastUpdated) until the current block.timestamp.

The issue is that since the acceptRateModelUpdate function doesn’t call accrue first, the next time a function that calls accrue is executed, the calculated interest will be based on the new rate model using the duration since the pool.lastUpdated, which could be a long time before the rate model was updated.

The acceptRateModelUpdate function should call accrue before updating the rate model to ensure that the interest calculation accurately reflects the old rate model up to the point of the update.

Sentiment Team: The issue was resolved in PR#223.

Proof of concept: PoC

Users can deposit the pool's respective asset by calling the deposit function in the pool contract. The function allows the pool owner to enforce a pool cap; however, when an ERC777 asset is used, this limit can be bypassed.

This issue arises because pool.totalAssets.assets is updated after the asset transfer, which is the point of reentrancy. A user can reenter with pool.totalAssets.assets not yet updated, so the limit check will use the old total asset value.

For example, if the totalAssets are 10 tokens away from the cap, a user could initially deposit 10 tokens. During the asset transfer, the user can reenter the function and make a second deposit of 10 tokens. As a result, totalAssets will end up being 10 tokens over the limit.

The asset transfer should be the initial step in the function to avoid a malicious state where tokens have not been transferred to the pool yet. Additionally, a nonReentrant modifier can be added to further secure the function.

Sentiment Team: The issue was resolved in PR#236.

The contract PositionManager inherits from PausableUpgradeable but does not implement the onlyOwner functions required to enable this functionality. As a result, the owner is unable to pause/unpause functions that have the whenNotPaused modifier.

Additionally, SuperPool contract inherits from Pausable contract, does not use the whenNotPaused or contains the onlyOwner functions.

To address this issue, it is recommended that pause and unpause functions be added to the PositionManager contract.

If SuperPool contract is not meant to have pause functionality, consider removing the inheritance from Pausable.

Sentiment Team: The issue was resolved in PR#242.

The _getPriceWithSanityChecks function is used in the ChainlinkEthOracle and ChainlinkUsdOracle contracts to validate the fetched price updates from the Chainlink feed. However, in the current implementation, the validation only reverts when the price is less than zero, meaning a price of zero would be considered valid.

This price is used to calculate position values and determine if positions can be liquidated. Consequently, positions could be incorrectly liquidated if a price of zero is returned instead of the oracle reverting.

Modify the price check to revert if the returned price is less than or equal to zero.

Sentiment Team: The issue was resolved in PR#254.

The reallocate function will move funds from one pool to another one. However, there is no check that the total amount of assets redeemed as effectively deposited into the new pools. Assets not deposited will not earn interest, so SuperPool user's earnings will be affected.

Verify that the total amount redeemed from pools matches the total amount deposited.

Sentiment Team: Acknowledged.

Pool is an upgradeable contract that inherits from ERC6909 contract. However, this contract don’t use storage gaps, which will result in a corrupted storage if a variable is added/removed.

Consider adding storage gaps in the ERC6909 contract.

Sentiment Team: Acknowledged.

Owner can reallocate funds by redeeming from certain pools and depositing into different pools. The issue arises when executing POOL.redeem as it uses assets instead of shares. Not only owner will redeem more assets than expected, but assets might not be fully deposited during the second loop as total assets redeemed will be greater that deposits total assets.

The withdraws array should contain shares instead of assets amount. Alternatively, calculate the shares that need to be redeemed with the asset amount passed and use that value in POOL.redeem().

Sentiment Team: The issue was resolved in PR#222.

If the owner of the SuperPool lost access to the feeRecipient wallet and tries to change it with the setFeeRecipient function it first accrues interest. This would lead to further loss in this case.

Update the feeRecipient before calling the accrue function.

Sentiment Team: Acknowledged.

The pool caps are not checked in the reallocate function. Therefore allocators can bypass the pool caps set by the owner of the SuperPool.

Check the pool caps in the reallocate function.

Sentiment Team: The issue was resolved in PR#231.

The ERC6909 contract is vulnerable to a well-known race condition in the approve function:

• User approves 100 tokens to a spender
• The user wants to increase the allowance to by 50 tokens and calls approve with 150 tokens
• Spender front runs the call and spends 100 tokens
• The user's approve call goes through and the allowance is set to 150 tokens
• Spender spends 150 tokens

Therefore the user wanted to allow the spender to spend 150 tokens but the spender was able to spend 250 tokens.

Implement increaseAllowance and decreaseAllowance functions.

Sentiment Team: The issue was resolved in PR#256.

In the redeem function of the SuperPool contract, it is not checked if the calculated assets amount from previewRedeem is 0. As previewRedeem rounds down it could be possible that a non-zero share amount is burned from the user but the user does not receive any assets in return.

Revert if the calculated assets amount is 0.

Sentiment Team: The issue was resolved in PR#241.

The PoolOwnerSet event in the Pool contract and the PoolAdded event in the SuperPool contract are not being emitted.

It is recommended to either remove the unused events or to use them in appropriate functions.

Sentiment Team: The issue was resolved in PR#257.

There are some typos in the codebase. PositionManager contract L30: “…liqudiator…” should be “…liquidator…” PositionManager contract L82: “SENIMENT” should be “SENTIMENT” RiskEngine contract L143: “…witihin…” should be “…within…”

We recommend updating the mentioned words in the codebase.

Sentiment Team: The issue was resolved in PR#258.

The registry variable in the RiskEngine contract is assigned in the constructor and there isn't any function for updating the 'registry' address in the contract.

Consider making the variable immutable.

Sentiment Team: The issue was resolved in PR#259.

In the borrow function of the Pool contract, the developer's comments state that minted shares should round up. However, the actual code rounds down because it uses convertToShares.

As rounding down is in favour of the borrower in this case. The excess debt is socialized among all other borrowers:

• Borrower borrowers X amount of funds
• The convertToShares function calculates that the given amount to borrow equals "Y + 0.9" debt

shares

• As solidity rounds down the user only gets Y debt shares
• Therefore the borrowed amount that equals 0.9 debt shares is socialized among all borrowers

Round up (against the borrower).

Sentiment Team: The issue was resolved in PR#245.

The protocol utilizes the Ownable library from OpenZeppelin, which does not include a 2-step ownership change implementation. This could potentially lead to undesired situations such as contracts being left without an owner after an incorrect update.

It is advisable to consider utilizing the Ownable2Step library.

Sentiment Team: Acknowledged.

In the accrue function of the Pool contract, the following lines are used twice:

// Store a timestamp for this accrue() call
// Used to compute the pending interest next time accrue() is called
pool.lastUpdated = uint128(block.timestamp);

Additionally, calling OwnableUpgradeable.__Ownable_init() right before the internal _transferOwnership(owner_) in the Pool contract and PositionManager contract is unnecessary, as the latter call will override the initialization calls.

Consider removing superfluous code to optimize the efficiency of the contracts.

Sentiment Team: The issue was resolved in PR#260.

Liquidations use the health check to validate if the position can be liquidated. If the price is stale, health check reverts, preventing the liquidation. Protocol might want to liquidate positions, no matter if price is stale, to avoid bad debt.

If this is not the intended behavior, consider bypassing the stale price check only for the liquidation process

Sentiment Team: Acknowledged.

The pool owner can remove a pool from the queue as long as there are no assets deposited. The _removeFromQueue function contains a loop that searches for the index of the pool to remove, and then removes the first item, shifting all subsequent items down.

While it is unlikely that the first loop will not find the poolId in the queue, toRemoveIdx may remain uninitialized, resulting in the removal of the first pool from the queue.

Implement an early return inside the first loop if the index is not found.

Sentiment Team: The issue was resolved in PR#261.

PositionManager allows users to batch actions using processBatch, with the condition that the position must be healthy when the transaction ends. This opens up the possibility of free Flash Loans, where users can borrow, transfer out the funds, use them outside of the protocol, and finally repay them to make sure the position is healthy.

Although this will considerably increase the gas cost of the transaction, health can be checked in every action to ensure users can't borrow without depositing collateral first.

Sentiment Team: Acknowledged.

_removePool has an onlyOwner modifier, although this is an internal function that can only be called from the setPoolCap which already has the access control.

Remove the onlyOwner modifier from the _removePool function.

Sentiment Team: The issue was resolved in PR#261.

Liquidators can get borrowers collateral at a discounted price. According to deployment script it will be 20%. Maximum seized collateral amount is calculated with this formula: debtRepaidValue.mulDiv((1e18 + LIQUIDATION_DISCOUNT), 1e18)

However, the correct way to implement this discount should be debtRepaidValue.mulDiv(1e18, (1e18

• - LIQUIDATION_DISCOUNT)) For example, getting $100 worth of asset with 20% discount means that

the user should pay 80$. But in the current implementation, the user gets $120 worth of asset by paying $100, effectively making the discount 16.66%.

Consider implementing the discount by decreasing the denominator instead of increasing the numerator. Otherwise, document this behavior as accepted.

Sentiment Team: The issue was resolved in PR#262.