Untangled Protocol Audit Report

Our auditing service includes the following two stages:

1. Pre-Audit Consulting Service
   • As a part of the pre-audit service, the Verilog team worked closely with the project development team to discuss potential vulnerability and smart contract development best practices. Verilog team is very appreciative of establishing an efficient and effective communication channel with the project team, as new findings are often exchanged promptly and fixes were deployed quickly, during the preliminary report stage.

2. Smart Contract Auditing Service
   • Verilog Solutions team analyzed the entire project using a detailed-oriented approach to capture the fundamental logic and suggested improvements to the existing code. Details can be found under Findings & Improvement Suggestions

• Code Assessment
  • We evaluate the overall quality of the code and comments as well as the architecture of the repository.
  • We help the project dev team improve the overall quality of the repository by providing suggestions on refactorization to follow the best practice of Web3 software engineering.

• Code Logic Analysis
  • We dive into the data structures and algorithms in the repository and provide suggestions to improve the data structures and algorithms for the lower time and space complexities.
  • We analyze the hierarchy among multiple modules and the relations among the source code files in the repository and provide suggestions to improve the code architecture with better readability, reusability, and extensibility.

• Business Logic Analysis
  • We study the technical whitepaper and other documents of the project and compare its specification with the functionality implemented in the code for any potential mismatch between them.
  • We analyze the risks and potential vulnerabilities in the business logic and make suggestions to improve the robustness of the project.

• Access Control Analysis
  • We perform a comprehensive assessment of the special roles of the project, including their authorities and privileges.
  • We provide suggestions regarding the best practice of privilege role management according to the standard operating procedures (SOP).

• Off-Chain Components Analysis
  • We analyze the off-chain modules that are interacting with the on-chain functionalities and provide suggestions according to the SOP.
  • We conduct a comprehensive investigation for potential risks and hacks that may happen on the off-chain components and provide suggestions for patches.

1. Inconsistency in the documentation and actual implementation

   Severity	High
   Source	pool/SecuritizationManager.sol#L51;
   Status	Resolved in commit a41e9eeef21ae4e603d044ed3621b1c1461cd911;

   • Description

     Anyone can create new pools while the Untangled documents state that only the approved entity can create new pools. A malicious pool can be created by an attacker to drain the user’s funds.

     1. In the Untangled Platform App User Guide, it states that only Originator/Issuer can create a new pool:

        “Business users who have been designated as Originator/Issuer can create a new pool under Portfolio/Pool”

     2. In the Untangled Whitepaper, it states that those who are approved by governance can create a new pool:

        “Anyone approved by Governance can create a Borrowing Pool …”

     However, in the code implementation, there is no access control on function SecuritizationManager.newPoolInstance() and anyone can create new pool instances by calling this function. Also, there is no check against who creates the pool in other components that interact with pools. Anyone can create a pool with his/her own desired settings for the pool(e.g. set the currency token of the pool or modify the pot address of the pool).

     Though attacking surfaces are limited because the pool implementation is set by admin roles. The code logic of the pool cannot be modified and only a new pool instance can be created. However, an attacker can create a pool with a malicious currency token. The pool and related contracts only work properly under the assumption that the currency token is a normal ERC20 token. Because of the malicious token breaking the assumption, the pool and all the related components will work improperly, which would put users’ funds at risk.

     Besides, based on the documents, all pools can only be created by trusted entities with verified ERC721 tokens representing real-world assets. Users might assume the pool created by the attacker is also a trusted pool and interact with it. Pool settings can be configured by the pool owner(attacker). An attacker can set the pot that is used to receive currency tokens with his/her own address or set an arbitrary interest rate.

   • Exploit Scenario

     Attacker Mallory creates a pool with non-verified ERC721 assets and the pool accepts USDC as currency. Mallory sets the pot that is used to collect currency tokens with his own address. User Alice assumes the pool created by Mallory is an official and trusted pool created by Untangled Finance team and interacts with this malicious pool. USDC tokens are transferred to the pot and Mallory withdraws all the USDC stored in the pot. Alice loses her funds.

   • Recommendations

     We recommend adding an access control for the SecuritizationManager.newPoolInstance() according to the design stated in the Untangled documents.

     Such an access control ensures that only designated roles or accounts can create new pool instances. With only pools created by trusted parties, the overall protocol safety level will be greatly improved.

   • Results

     Resolved in commit a41e9eeef21ae4e603d044ed3621b1c1461cd911. Untangled Finance team added a new role POOL_CREATOR and only POOL_CREATEOR can create a new pool instance.

     A new privileged role was introduced in this fix commit. We recommend modifying the instructions and explanations on Untangled documents accordingly.

2. Lack of access control on critical functions

   Severity	High
   Source	protocols/note-sale/crowdsale/TimedCrowdsale.sol#L45;
   Status	Resolved in commit d570a6e950d6c0ab641ece08fc657a5ccad606ee;

   • Description

     There is no access control on a function that can change critical state variables. Anyone can call this function to affect the status of the sale round.

     Function TimedCrowdsale.newSaleRoundTime() sets openingTime and closingTime of current sale round. They are two very critical state variables that can determine the status of the current sale round (whether the sale is open or closed). Ideally, only owner roles can extend the sale round by calling the function TimedCrowdsale.extendTime(), which is a function that only can be called by OWENR_ROLE. However, anyone can just call TimedCrowdsale.newSaleRoundTime() to bypass the OWNER_ROLE check and reset theclosingTime.

     Any user can determine the status of the sale, which violates the design that only privileged roles like OWNER_ROLE can modify the status of the sale round. Users can just stop or reopen or extend the sale round themselves by calling this function. Token sales will be interrupted and sales amount will be unexpected, which can have a great impact on pool shares distribution and hurt token holders’ interest.

   • Exploit Scenario

     The token sale ends and attacker Mallory sets the closingTime through the public function TimedCrowdsale.newSaleRoundTime() to reopen the sale and buy more tokens. More tokens are sold than expected, which dilutes the existing token holder’s share and value.

   • Recommendations

     Add access control to function TimedCrowdsale.newSaleRoundTime().

     Access control on this function ensures that only approved roles have the ability to modify the two critical variables through this function. Token sales status can no longer be modified by unauthorized users.

   • Results

     Resolved in commit d570a6e950d6c0ab641ece08fc657a5ccad606ee. An onlyRole modifier has been added to the function.

     Same as functionTimedCrowdsale.extendTime(), TimedCrowdsale.newSaleRoundTime() can only be called by OWNER_ROLE. Sale round status can only be modified by OWNER_ROLE, no more unexpected interruptions on token sales from unauthorized users.

3. Inconsistency in the function name and actual implementation

   Severity	High
   Source	protocols/note-sale/crowdsale/Crowdsale.sol#L108;
   Status	Resolved in commit d570a6e950d6c0ab641ece08fc657a5ccad606ee;

   • Description

     The name of function getTimeStartEarningInterest() indicates that it should return time starts earning interest instead of block.timestamp. Incorrect implementation eventually affects the calculation of the long sale token amount that the user can buy.

     According to the name of function Crowdsale.getTimeStartEarningInterest(), it should return the starting earning interest time. However, in the actual implementation, this function returns block.timestamp. This function is used in getLongSaleTokenPrice() to return the long sale token price, which is eventually used in the calculation of the long sale token amount that a user can buy. The getLongSaleTokenPrice() reverts if getTimeStartEarningInterest() returns zero value and user will not be able to buy tokens.

     Under current implementation, getTimeStartEarningInterest() will always return block.timestamp which is always non-zero, thus it will never cause require statement in getLongSaleTokenPrice() to revert. There will be no difference with or without the check.

     The wrong implementation of the function located in the base contract will affect the proper operations of all other functions that rely on it. Under current code implementation, users are able to buy tokens regardless of the time starts earning interest that Untangled protocol wishes to set. An unexpected token amount will be sold to users without the correct setting on the starting earn interest time.

   • Exploit Scenario

     The time starts earning interest cannot be set and user Alice can always buy tokens from the token sale contracts regardless of the actual start earning interest time that Untangled protocol wishes to set. Alice ends up obtaining more note tokens than she should.

   • Recommendations

     Check and rewrite the implementation code of function Crowdsale.getTimeStartEarningInterest().

     Correct implementation can ensure other functions that rely on this function works properly.

   • Results

     Resolved in commit d570a6e950d6c0ab641ece08fc657a5ccad606ee. function Crowdsale.getTimeStartEarningInterest() was removed from base contract Crowdsale.sol and was replaced by the variable timeStartEarningInterest exists in the contract MintedIncreasingInterestTGE.sol and MintedNormalTGE.sol, which would only have limited effect inside the contract and contracts that inherit it.

     The value of timeStartEarningInterest can be set inside function setupLongSale(). The require statement will revert if the value is not set correctly. Variable timeStartEarningInterest works properly as expected.

4. Inconsistency in business logic and actual implementation

   Severity	High
   Source	pool/base/SecuritizationPoolServiceBase.sol#L18, #L32;
   Status	Resolved in commit a2909c0ac25e76953fa3fd0ee42b667619e8cf23;

   • Description

     Functions used to convert token values are implemented with incorrect equations. It would result in incorrect calculation of note token values and users’ share proportion in the pool, which directly affects the proper operations of the whole protocol.

     SecuritizationPoolServiceBase.convertTokenValueToCurrencyAmount() and SecuritizationPoolServiceBase.convertCurrencyAmountToTokenValue() convert token values from one token to another token by comparing decimal differences of the two tokens. However, the equations used to convert token values are incorrect. converted token value should be 10**(bigger_decimal- smaller_decimal) rather than 10**bigger_decimal- smaller_decimal .

     These two functions are used in various important operations used for calculating token price and token value such as DistributionAssessor.getSOTTokenPrice() and SecuritizationPoolValueService.getExpectedERC20AssetValue(). It eventually affects the calculation of note token value and users’ share proportion in the pool. Incorrect implementation of these two critical functions can result in incorrect calculations of the number of all the users’ funds and yields and loans in the pool.

   • Exploit Scenario

     User Alice’s note token value is calculated incorrectly and it’s far less than it should be.

   • Recommendations

     Change the function implementation to the following suggestions:

     function convertTokenValueToCurrencyAmount(
             address pool,
             address tokenAddress,
             uint256 tokenValue
         ) internal view returns (uint256) {
             uint256 currencyDecimals = ERC20(ISecuritizationPool(pool).underlyingCurrency()).decimals();
             uint256 tokenDecimals = ERC20(tokenAddress).decimals();
     
             return
                 currencyDecimals > tokenDecimals
                     ? tokenValue * (10**(currencyDecimals - tokenDecimals))
                     : tokenValue / (10**(tokenDecimals - currencyDecimals));
         }
     
         function convertCurrencyAmountToTokenValue(
             address pool,
             address tokenAddress,
             uint256 currencyAmount
         ) internal view returns (uint256) {
             uint256 currencyDecimals = ERC20(ISecuritizationPool(pool).underlyingCurrency()).decimals();
             uint256 tokenDecimals = ERC20(tokenAddress).decimals();
     
             return
                 currencyDecimals > tokenDecimals
                     ? currencyAmount / (10**(currencyDecimals - tokenDecimals))
                     : currencyAmount * (10**(tokenDecimals - currencyDecimals));
         }

   • Results

     Resolved in commit a2909c0ac25e76953fa3fd0ee42b667619e8cf23. Token value conversions are now calculated with correct equations.

5. Inconsistency in business logic and actual implementation

   Severity	High
   Source	pool/DistributionAssessor.sol#L13;
   Status	Resolved in commit 0ee18c763347a7ade21528d47b7cfc765fda0be9;

   • Description

     getSOTTokenPrice() only returns the accumulated interest, without adding the default initial price. Incorrect SOT token price causes the currency redemption amount calculation incorrect and users’ shares of the pool are distributed unfairly.

     SOT token price consists of two parts. It increases from the default price and it will be bigger than the default token price with accumulated interest. However, function DistributionAssessor.getSOTTokenPrice() directly returns the interest part calculated by function _calculateInterestForDuration(). The token default price is not added on top of the calculated interest.

     This incorrect calculation results in all the calculated SOT token prices being smaller than they should be. SOT token price stands for the share proportion of users owned in the pool. Users can redeem more funds than they should. Users who redeem early can redeem more funds than they should and there might not be enough funds for users who redeem late. This issue puts funds in the pool at great risk.

   • Exploit Scenario

     User Alice makes the redeem request while the pool still has enough funds and she receives more tokens than she should.

   • Recommendations

     Add the initial token price to the return value of the cumulated interest in function getSOTTokenPrice() to make sure token prices are calculated correctly and funds in the pool are distributed fairly to note token holders.

     function getSOTTokenPrice(address pool, uint256 timestamp) public view override returns (uint256) {
             ISecuritizationPool securitizationPool = ISecuritizationPool(pool);
             ERC20 noteToken = ERC20(securitizationPool.sotToken());
     
             if (address(noteToken) == address(0) || noteToken.totalSupply() == 0) return 0;
             uint256 ONE_SOT_DEFAULT_PRICE = convertTokenValueToCurrencyAmount(
                 address(securitizationPool),
                 address(noteToken),
                 1 * 10**uint256(noteToken.decimals())
             );
             uint256 openingBlockTimestamp = securitizationPool.openingBlockTimestamp();
     
             if (timestamp < openingBlockTimestamp) return ONE_SOT_DEFAULT_PRICE;
     
             uint32 interestRateSOT = securitizationPool.interestRateSOT();
     
             return ONE_SOT_DEFAULT_PRICE + _calculateInterestForDuration(ONE_SOT_DEFAULT_PRICE, interestRateSOT, timestamp - openingBlockTimestamp);
         }

   • Results

     Resolved in commit 0ee18c763347a7ade21528d47b7cfc765fda0be9. ONE_SOT_DEFAULT_PRICE is added on top of the return value of _calculateInterestForDuration(). SOT token prices are calculated correctly.

1. Inconsistency in business logic and actual implementation

   Severity	Medium
   Source	pool/SecuritizationPoolValueService.sol#L36, #L122;
   Status	Resolved in commit b69d2226bd47b296e80b86c309262f6086efb34c;

   • Description

     Expected asset values are calculated incorrectly due to incorrect calculation of total debt. It would affect users and frontend implementation when querying information with this function.

     Function SecuritizationPoolValueService.getExpectedAssetValue() and SecuritizationPoolValueService.getExpectedERC20AssetValue() are two functions which calculate the expected ERC721/ERC20 asset value. These two functions should have similar logic on implementation. Expected value are calculated with total debt and present value. However, the totalDebt in SecuritizationPoolValueService.getExpectedAssetValue() is calculated incorrectly. It should be calculated with timestamp, instead of expirationTimestamp.

     When the debt is overdue, the computed user’s debt will be smaller than expected. When the debt is not overdue, the computed user’s debt will be bigger than expected. In Either way, the user’s debt is not calculated correctly, thus it will affect the calculation of the expected asset value.

     Public functiongetExpectedAssetValue() is currently only used in an external view function and not referenced by any other contracts. Incorrect asset value will be given when querying from the protocol, which might affect the frontend implementation. Pool funds will not be affected by the incorrect calculation.

   • Exploit Scenario

     Incorrect expected asset values are given to users or used in the frontend when querying this function.

   • Recommendations

     Calculate the total debt with timestamp instead of expirationTimestamp.

     function getExpectedAssetValue(
             address poolAddress,
             address tokenAddress,
             uint256 tokenId,
             uint256 timestamp
         ) public view returns (uint256) {
             IUntangledERC721 loanAssetToken = IUntangledERC721(tokenAddress);
             uint256 expirationTimestamp = loanAssetToken.getExpirationTimestamp(tokenId);
     
             uint256 overdue = timestamp > expirationTimestamp ? timestamp - expirationTimestamp : 0;
             uint256 totalDebt = loanAssetToken.getTotalExpectedRepaymentValue(tokenId, timestamp);
     
             uint256 presentValue = getPresentValueWithNAVCalculation(
                 poolAddress,
                 totalDebt,
                 loanAssetToken.getInterestRate(tokenId),
                 loanAssetToken.getRiskScore(tokenId),
                 overdue,
                 loanAssetToken.getAssetPurpose(tokenId)
             );
     
             if (timestamp < expirationTimestamp) {
                 totalDebt = loanAssetToken.getTotalExpectedRepaymentValue(tokenId, timestamp);
             }
     
             return presentValue < totalDebt ? presentValue : totalDebt;
         }

   • Results

     Resolved in commit b69d2226bd47b296e80b86c309262f6086efb34c. totalDebt is now calculated correctly with timestamp.

2. Lack of state assertion on critical functions

   Severity	Medium
   Source	protocols/loan/LoanInterestTermsContract.sol#L134;
   Status	Resolved in commit 0857c34e2cf88e447205d7b779a64196c35dd0c5;

   • Description

     Missing loan status check on function LoanInterestTermsContract.registerRepayment() allows loan repayments to be made on loans that are already concluded or not started. Unnecessary Repayments on inadequate loans can interfere the loan management and cause loss of user funds.

     LoanInterestTermsContract.registerRepayment() is called in function LoanRepaymentRouter._doRepay() to help users to make loan repayments. However, both of the functions miss the loan status check. Loan Repayments can be made on loans that are already concluded or not started.

     Loans that are concluded or not started should not be allowed for accepting repayments. Loan repayments on loans with incorrect status allow users to make unnecessary loan repayments, thus it results in loss of funds.

   • Exploit Scenario

     Alice accidentally makes a repayment on the loan that has already concluded. She lost the part of funds that are used to make repayments.

   • Recommendations

     Add loan status check on LoanInterestTermsContract.registerRepayment() to filter out loans with incorrect status.

     It can prevent loss of funds due to repayments on inadequate loans.

   • Results

     Resolved in commit 0857c34e2cf88e447205d7b779a64196c35dd0c5. A require statement for loan status check has been added to function LoanInterestTermsContract.registerRepayment().

3. Inconsistency in the function name and actual implementation

   Severity	Medium
   Source	protocols/note-sale/fab/NoteTokenFactory.sol#L52, #59
   Status	Resolved in commit 9ed09bcf4eb60e975ec9cbc403f2fb9d5bab5a26;

   • Description

     The function name indicates it should be able to pause and unpause tokens. However, they can only pause tokens and tokens cannot be unpaused once they are paused. The system can not go back to normal operations once tokens are paused.

     As the names of function NoteTokenFactory.pauseUnpauseToken() and NoteTokenFactory.pauseUnpauseAllTokens() indicates, it should be able to pause and unpause tokens. However, under the current implementation, they can only be used to pause tokens. Paused tokens can result in failures of protocol operations which are heavily dependent on the note tokens. And there are no other functions that are designed to unpause the paused token. This critical operation is irreversible and will result in the failure of the whole protocol.

     function pauseUnpauseToken(address tokenAddress) external whenNotPaused nonReentrant onlyRole(DEFAULT_ADMIN_ROLE) {
     	require(isExistingTokens[tokenAddress], 'NoteTokenFactory: token does not exist');
     	NoteToken token = NoteToken(tokenAddress);
     	if (token.paused()) token.unpause(); // redundant unpause
     	token.pause();
     }
     function pauseUnpauseAllTokens() external whenNotPaused nonReentrant onlyRole(DEFAULT_ADMIN_ROLE) {
     	for (uint256 i = 0; i < tokens.length; i++) {
     		if (tokens[i].paused()) tokens[i].unpause();
     		else tokens[i].pause();
     	}
     }

   • Exploit Scenario

     Admin accidentally calls the function to pause all tokens and the whole system cannot work properly because of the paused note tokens.

   • Recommendations

     Rewrite these two functions, so they can pause or unpause tokens.

   • Results

     Resolved in commit 9ed09bcf4eb60e975ec9cbc403f2fb9d5bab5a26. These two functions are available to pause and unpause tokens.

1. Mismatched array length

   Severity	Low
   Source	pool/SecuritizationPool.sol#L189, #L219;
   Status	Resolved in commit 0e12cc6adf8eea62639a823ec8284334cb1ee2a6;

   • Description

     The number of times the for loop would run in withdrawAssets() and withdrawERC20Assets() functions are based on one of its input array lengths, therefore, if the decisive parameter is input wrong, these two functions can still run without error but not as the user expected.

     Shown as the code below, if the for loop decisive parameter tokenIds is input wrong by mistake, the recipients will not be able to receive their tokens. And more importantly, if the tokenIds and recipients is mismatch, token will be transferred to the wrong recipient.

     function withdrawAssets(
             address[] calldata tokenAddresses,
             uint256[] calldata tokenIds,
             address[] calldata recipients
         ) external override whenNotPaused nonReentrant onlyRole(OWNER_ROLE) {
             for (uint256 i = 0; i < tokenIds.length; i++) {
                 require(removeNFTAsset(tokenAddresses[i], tokenIds[i]), 'SecuritizationPool: Asset does not exist');
                 IUntangledERC721(tokenAddresses[i]).safeTransferFrom(address(this), recipients[i], tokenIds[i]);
             }
         }

   • Exploit Scenario

     N/A

   • Recommendations

     Check if the input arrays’ lengths are equal.

   • Results

     Resolved in commit 0e12cc6adf8eea62639a823ec8284334cb1ee2a6. a require statement was added to confirm the length of the three input arrays is equal.

2. Inconsistency in variable names and actual implementation

   Severity	Low
   Source	tokens/ERC721/invoice/AcceptedInvoiceToken.sol#L99;
   Status	Resolved in commit b5d7c9fb3283902bccc8ace0210a8d7495021554;

   • Description

     Fiat amount instead of the repaid amount is updated when a loan is not fully repaid and repaid amount only gets updated upon full repayments. Updates of these two state variables are confusing and do work in the way that their variable names indicate.

     The repaid amount of the AIT token only gets updated upon the full repayment of the loan. When a loan is not fully repaid, the fiat amount instead of the repaid amount is updated.

     The way how these two variables get updated does not work as their names indicate. Fiat amount should be a variable that shows how much the total loan amount is in total and it should stay unchanged on repayments. The repaid amount should be a variable that keeps track of how many repayments users have made. Right now, the fiat amount gets deducted every time when a repayment is made and repaid amount only gets updated upon full repayments. Decreasing of fiat amount loses its function to record the total loans that users owe. The names of these two variables are confusing and do not reflect how they work in the protocol.

   • Exploit Scenario

     Protocol fails to obtain the total loan data due to the deduction on fiat amount every time a repayment is made.

   • Recommendations

     Update the repay amount whenever the repayment is made Instead of the fiat amount. In this way, the system can keep a correct track of repaid amounts and total fiat amounts.

   • Results

     Resolved in commit b5d7c9fb3283902bccc8ace0210a8d7495021554. The paid amount fiat amount gets update upon repayments. Current fix is:

     if (fiatAmountRemain == 0) {
         metadata.paidAmount += payAmounts[i];
         super._burn(tokenIds[i]);
     } else {
         metadata.paidAmount += payAmounts[i];
     }

     The code can be optimized:

     metadata.paidAmount += payAmounts[i];
     if (fiatAmountRemain == 0) {
         super._burn(tokenIds[i]);
     }

3. Lack of function virtual modifier

   Severity	Low
   Source	protocols/note-sale/crowdsale/Crowdsale.sol;
   Status	Resolved in commit b7bb0e71577d153766452d83968b06f4638ab8fe;

   • Description

     The function names do not reflect their actual behaviors. It confuses both users and future developers. After deep dive into the inheritance structure, we think the function shown below should be virtual .

     • isLongSale() returns false, which should return a correct status of whether this crowd sale is a long sale. If this function is not meant to be implemented in the crowd sale base class, mark it as virtual and implement it later. Otherwise, remove isLongSale() in Crowdsale.sol

     • getLongSaleTokenAmount() returns the rate state variable. The rate state variable indicates the exchange rate between selling token <> stable coins (with 10^4 decimals), but the function name indicates it returns the token amount of the sale. If this function is not meant to be implemented in the crowd sale base class, mark it as virtual and implemented it later. Otherwise, remove getLongSaleTokenAmount() in Crowdsale.sol
   • Exploit Scenario

     N/A

   • Recommendations

     Mark the base class function as a virtual

   • Results

     Resolved in commit b7bb0e71577d153766452d83968b06f4638ab8fe.

4. Unused functions

   Severity	Low
   Source	pool/DistributionOperator.sol#L76; pool/SecuritizationPool.sol#L351, L362; protocols/loan/LoanRegistry.sol#L133;
   Status	Resolved in commit 9ed09bcf4eb60e975ec9cbc403f2fb9d5bab5a26;

   • Description

     • function DistributionOperator._distributeJOTInBatch() is never used.

     • SecuritizationPool.increasePaidInterestAmountSOT() and SecuritizationPool.increasePaidPrincipalAmountSOT() are two functions only can be called by onlyDistributionOperator(). But they are not called by DistributionOperator.

     • Function setCompletedLoan is defined as onlyLoanKernel, but the contractLoanKernel does not call it.

     Unused functions waste gas when deploying the contract.

   • Exploit Scenario

     N/A

   • Recommendations

     Remove unused functions.

   • Results

     Resolved in commit 9ed09bcf4eb60e975ec9cbc403f2fb9d5bab5a26. Unused functions have been removed.

5. Unreachable Code

   Severity	Low
   Source	protocols/loan/LoanInterestTermsContract.sol#L175;
   Status	Resolved in commit fc46523db3fb60ab655ff2ed406db023d2498f00;

   • Description

     There is a if-else path that can never be reached.

     When expectedInterest == 0, the statement unitsOfRepayment < expectedPrincipal will always be true because unitsOfRepayment < expectedPrincipal + expectedInterest. So the path where is stated above can never be reached. This part of code can be removed.

     if (unitsOfRepayment >= expectedPrincipal + expectedInterest) {
         _setCompletedRepayment(agreementId);
         _addRepaidInterestAmount(agreementId, expectedInterest);
         _addRepaidPrincipalAmount(agreementId, expectedPrincipal);
         remains = unitsOfRepayment - (expectedPrincipal + expectedInterest);
     } else {
         if (expectedInterest == 0) {
     				// == Start of Unreachable Code ==
     				// since unitsOfRepayment < expectedPrincipal + expectedInterest
     				// && expectedInterest == 0
     				// so unitsOfRepayment < expectedPrincipal is for sure
             if (unitsOfRepayment >= expectedPrincipal) {
                 _addRepaidPrincipalAmount(agreementId, expectedPrincipal);
                 remains = unitsOfRepayment - expectedPrincipal;
             }
     				// == End of Unreachable Code ==
     				else {
                 _addRepaidPrincipalAmount(agreementId, unitsOfRepayment);
             }
     }

   • Exploit Scenario

     N/A

   • Recommendations

     Remove the unreachable code stated above.

   • Results

     Resolved by removing the unreachable code in commit fc46523db3fb60ab655ff2ed406db023d2498f00.

6. Redundant code

   Severity	Low
   Source	pool/SecuritizationPool.sol; pool/DistributionTranche.sol; protocols/loan/LoanInterestTermsContract.sol; protocols/note-sale/MintedIncreasingInterestTGE.sol;
   Status	Resolved in commit 7d2e07e06c3de02ac39a9a880228ac0bbf185d93;

   • Description

     Functions that have no external calls / only atomic operation need no Reentrancy Guard. Unnecessary Reentrancy Guard wastes gas.

     1. SecuritizationPool.sol increaseLockedDistributeBalance() no external calls

     2. DistributionTranche.sol redeemToken() only atomic operation

     3. LoanInterestTermsContract.sol registerTermStart() no external calls + only atomic operation

     4. SecuritizationPool.sol setupRiskScores no external calls

     5. MintedIncreasingInterestTGE.sol setYield() no external calls
   • Exploit Scenario

     N/A

   • Recommendations

     Remove no need Reentrancy Guard to save gas.

   • Results

     Resolved in commit 7d2e07e06c3de02ac39a9a880228ac0bbf185d93.

1. Floating solidity pragma version

   Severity	Informational
   Source	Untangle protocol
   Status	Acknowledged

   • Description

     Current smart contracts use ^0.8.0. And compilers within versions≥ 0.8.0 and <0.9.0 can be used to compile those contracts. Therefore, the contract may be deployed with a newer or latest compiler version which generally has higher risks of undiscovered bugs.

     It is a good practice to fix the solidity pragma version if the contract is not designed as a package or library that will be used by other projects or developers.

     Reference: SWC-103.

   • Exploit Scenario

     N/A

   • Recommendations

     Use the fixed solidity pragma version.

   • Results

     Acknowledged. The untangled team acknowledged that the solidity version should be fixed. However, for the convenience of future upgrades, they decided that just set the solidity compiler version in hardhat config files.

2. State variable initialization in the wrong place

   Severity	Informational
   Source	protocols/note-sale/crowdsale/Crowdsale.sol;
   Status	Resolved in commit 9ed09bcf4eb60e975ec9cbc403f2fb9d5bab5a26;

   • Description

     State variables defined in crowdsale.sol are initialized in MintedNormalTGE.sol and MintedIncreasingInterestTGE.sol. The misplaced initialization confuses users and future developers.

     • registry: defined in crowdsale.sol but initialized in MintedNormalTGE.soland MintedIncreasingInterestTGE.

     • pool: defined in crowdsale.sol but initialized in MintedNormalTGE.soland MintedIncreasingInterestTGE.

     • token: defined in crowdsale.sol but initialized in MintedNormalTGE.soland MintedIncreasingInterestTGE.

     • currency: defined in crowdsale.sol but initialized in MintedNormalTGE.soland MintedIncreasingInterestTGE.
   • Exploit Scenario

     N/A

   • Recommendations

     Initialize the state variables in the class where it was defined.

   • Results

     Resolved in commit 9ed09bcf4eb60e975ec9cbc403f2fb9d5bab5a26.

3. Naming issues

   Severity	Informational
   Source	Untangled protocol
   Status	Resolved in commit f83a44d8fc6be599c9ee3f68877ad0f855d500ae;

   • Description

     The coding style is inconsistent. Inconsistent coding style reduces code readability.

     Some private/internal function names have underscore prefixes and some don’t. Therefore, co-developers may use the wrong function because of the Inconsistent coding style and has a bigger chance to write an insecure smart contract.

   • Exploit Scenario

     N/A

   • Recommendations

     We suggest all function names of private/internal functions contain an underscore as a prefix for better readability and coding style practice.

   • Results

     Resolved in commit f83a44d8fc6be599c9ee3f68877ad0f855d500ae.

4. Non-descriptive error message

   Severity	Informational
   Source	protocols/loan/LoanKernel.sol#L18; protocols/loan/LoanInterestTermsContract.sol#L79;
   Status	Resolved in commit 9ed09bcf4eb60e975ec9cbc403f2fb9d5bab5a26 and commit 317baf65131faf60d9c97d86066d02760f896907;

   • Description

     Shown as code below, the error message provided byvalidFillingOrderAddresses is unclear. Therefore, it is difficult for a user or developer to locate the error when the unclear error messages occurs.

     modifier validFillingOrderAddresses(address[] memory _orderAddresses) {
       require(_orderAddresses[uint8(FillingAddressesIndex.CREDITOR)] != address(0x0), 'PARAM1');
       require(_orderAddresses[uint8(FillingAddressesIndex.REPAYMENT_ROUTER)] != address(0x0), 'PARAM4');
       require(_orderAddresses[uint8(FillingAddressesIndex.TERM_CONTRACT)] != address(0x0), 'PARAM5');
       require(_orderAddresses[uint8(FillingAddressesIndex.PRINCIPAL_TOKEN_ADDRESS)] != address(0x0), 'PARAM2');
       _;
     }

     modifier onlyHaventStartedLoan(bytes32 agreementId) {
         require(!startedLoan[agreementId], 'LOAN1');
         _;
     }

   • Exploit Scenario

     N/A

   • Recommendations

     Use descriptive error message.

     "LoanKernel: orderAddresses[CREDITOR] is zero address."

     Or provide an error message reference table, so one can find the detailed explanation with the error message code.

   • Results

     Resolved in commit 9ed09bcf4eb60e975ec9cbc403f2fb9d5bab5a26 and commit 317baf65131faf60d9c97d86066d02760f896907.

5. Lack of state assertion on critical functions

   Severity	Informational
   Source	protocols/note-sale/base/LongSaleInterest.sol#L20;
   Status	Resolved in commit 9ed09bcf4eb60e975ec9cbc403f2fb9d5bab5a26;

   • Description

     Inside function LongSaleInterest.getPurchasePrice(), it subtract _durationLengthInSec from _termLengthInSeconds and the purchase price is base on this subtract operation. It may fail with subtraction underflow and leads to an incorrect price.

   • Exploit Scenario

     N/A

   • Recommendations

     We recommend adding a require function to require _termLengthInSeconds not smaller than _durationLengthInSec with an error message.

   • Results

     Resolved in commit 9ed09bcf4eb60e975ec9cbc403f2fb9d5bab5a26.

6. Unused variables

   Severity	Informational
   Source	protocols/note-sale/crowdsale/Crowdsale.sol#L33; pool/SecuritizationPool.sol#L120; tokens/ERC721/invoice/AcceptedInvoiceToken.sol#L163;
   Status	Resolved in commit f83a44d8fc6be599c9ee3f68877ad0f855d500ae and commit b20946fe0f24a8528e6574d456d5f5e7a0cc34e4;

   • Description

     • State variable unused:
       • Crowdsale.solL33 initialized.

     • Function parameter unused:
       • SecuritizationPool.sol L120 operator, from, data ;
       • AcceptedInvoiceToken.sol L163 timestamp.

     these unused parameters and state variables waste gas.

   • Exploit Scenario

     N/A

   • Recommendations

     1. Remove unused state variables.

     2. Remove unused parameters, or add placeholders to mute the warnings:

     function getExpectedRepaymentValues(uint256 tokenId, uint256 timestamp) public view returns (uint256, uint256) {
     	timestamp; // placeholder
     	return (entries[bytes32(tokenId)].fiatAmount - entries[bytes32(tokenId)].paidAmount, 0);
     }

   • Results

     Resolved in commit f83a44d8fc6be599c9ee3f68877ad0f855d500ae and commit b20946fe0f24a8528e6574d456d5f5e7a0cc34e4.