(code-423n4/2022-03-lifinance-findings)
Source, Code

Number: 10
Problem: Success even if contract no exist
Failed transfer with low level call won't revert
Severity: Medium

Impact:
swap is used throughout the code via _executeSwaps in Swapper.sol. According to Solidity Docs the call may return true even if it was a failure. This may result in user funds lost because funds were transferred into this contract in preparation for the swap. The swap fails but doesn't revert. There is a way this can happen through GenericSwapFacet.sol due to a missing require that is present in the other facets which is a separate issue but gives this issue more relevance.

Proof of concept:
1. Alice uses Generic swap with 100 DAI
2. Alice's 100 DAI are sent to the Swapper.sol contract
3. The call on swap _swapData.callTo.call{ value: msg.value }(_swapData.callData); fails but returns success due to nonexisting contract
4. postSwapBalance = 0
5. Alice receives nothing in return

Mitigation:
Check for contract existence

#CODE4ARENA #MEDIUM #REPORT #CALL #LIFINANCE

(code-423n4/2022-03-lifinance-findings)
Source, Code

Number: 11
Problem: Use call instead of transfer and send
Use of transfer() may lead to failures
Severity: Medium

Proof of Concept:
src/Facets/WithdrawFacet.sol
31: payable(sendTo).transfer(_amount);

When withdrawing native token, the withdraw is being handled with a payable.transfer() call.
This is unsafe as transfer has hard coded gas budget and can fail.
Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.

Mitigation:
Use call() instead, without hardcoded gas limits along with checks-effects-interactions pattern or reentrancy guards for reentrancy protection.

#CODE4ARENA #MEDIUM #REPORT #GAS #LIFINANCE

(code-423n4/2022-01-yield-findings)
Source

Number: 13
Problem: Oracle data feed is insufficiently validated.
Severity: Medium

Impact: Price can be stale(out-of-date) and can lead to wrong quoteAmount return value

Proof of concept:
Oracle data feed is insufficiently validated. There is no check for stale price and round completeness.
Price can be stale and can lead to wrong quoteAmount return value

#CODE4ARENA #MEDIUM #REPORT #ORACLE #YIELD_CONVEX

(code-423n4/2022-03-prepo-findings)
Source, Code

Number: 14
Problem: The first depositor can manipulate rewards. First depositor can break minting of shares
Severity: Medium

Impact: The attack vector and impact is the same as TOB-YEARN-003, where users may not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large “donation”.

Proof of Concept:
- Attacker deposits 2 wei (so that it is greater than min fee) to mint 1 share
- Attacker transfers exorbitant amount to _strategyController to greatly inflate the share’s price. Note that the _strategyController deposits its entire balance to the strategy when its deposit() function is called.
- Subsequent depositors instead have to deposit an equivalent sum to avoid minting 0 shares. Otherwise, their deposits accrue to the attacker who holds the only share.

Mitigation:
- Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case i.e. when totalSupply() == 0, send the first min liquidity LP tokens to the zero address to enable share dilution.
- Ensure the number of shares to be minted is non-zero: require(_shares != 0, "zero shares minted");
- Create a periphery contract that contains a wrapper function that atomically calls initialize() and deposit()
- Call deposit() once in initialize() to achieve the same effect as the suggestion above.

#CODE4ARENA #MEDIUM #REPORT #SHARES #PREPO

(code-423n4/2022-03-timeswap-findings)
Source, Code

Number: 15
Problem: Underflown variable in borrowGivenDebtETHCollateral function
Severity: Medium

Impact:
- borrowGivenDebtETHCollateral function does never properly call ETH.transfer due to underflow.
- If borrowGivenDebtETHCollateral function is not deprecated, it would cause unexpected behaviors for users.

Proof of Concept:
Here are codes which contain a potential issue.
if (maxCollateral > dueOut.collateral) {
uint256 excess;
unchecked {
excess -= dueOut.collateral;
}
ETH.transfer(payable(msg.sender), excess);
}
excess variable is uint256, and dueOut.collateral variable is uint112 as shown below. Hence, both variables will never be less than 0.

struct Due {
uint112 debt;
uint112 collateral;
uint32 startBlock;
}
uint256 excess is initialized to 0. However, subtracting dueOut.collateral variable which is more than or equal to 0 from excess variable which is 0 will be less than 0. Hence, excess -= dueOut.collateral will be less than 0, and excess will be underflown.

Mitigation:
The code should properly initialize excess variable.
borrowGivenPercentETHCollateral function uses uint256 excess = maxCollateral at similar functionality.
https://github.com/code-423n4/2022-03-timeswap/blob/main/Timeswap/Convenience/contracts/libraries/Borrow.sol#L347
Hence, just initializing excess variable with maxCollateral can be a potential workaround to prevent the underflown.

if (maxCollateral > dueOut.collateral) {
uint256 excess = maxCollateral;
unchecked {
excess -= dueOut.collateral;
}
ETH.transfer(payable(msg.sender), excess);
}

#CODE4ARENA #MEDIUM #REPORT #UNDERFLOW_OVERFLOW #TIMESWAP

(code-423n4/2022-03-timeswap-findings)
Source, Code

Number: 16
Problem: DoS by repay with underflow. The pay() function can still be DOSed
Severity: Medium

Impact:
in the pay() function users repay their debt and in line 364:
it decreases their debt.
lets say a user wants to repay all his debt, he calls the pay() function with his full debt.
an attacker can see it and frontrun to repay a single token for his debt (since it's likely the token uses 18 decimals, a single token is worth almost nothing)
and since your solidity version is above 0.8.0 the line:
due.debt -= assetsIn[i]; will revert due to underflow

The attacker can keep doing it everytime the user is going to pay and since 1 token is baisicly 0$ (18 decimals) the attacker doesn't lose real money
The pay() function however is still DOSable. Having the Convenience contract contain a workaround means the Convenience contract is no longer a convenience but a requirement.
due.debt -= param.assetsIn[i];
A DoS on every user that repay his full debt (or enough that the difference between his total debt to what he pays his negligible)

Mitigation:
if assetsIn[i] is bigger than due.debt set assetsIn[i]=due.debt and due.debt=0

#CODE4ARENA #MEDIUM #REPORT #DOS #TIMESWAP

(code-423n4/2022-03-timeswap-findings)
Source, Code

Number: 17
Problem: NPM Dependency confusion. Unclaimed NPM Package and Scope/Org
Severity: Medium

Impact:
I discovered an npm package and the scope of the package is unclaimed on the NPM website. This will give any User to claim that package and be able to Upload a Malicious Code under that unclaimed package. This results in achieving the Remote code execution on developers/users' machine who depends on the timeswap repository to build it on local env.
##Vulnerable Package Name: @timeswap-labs/timeswap-v1-core

Proof of Concept:
1) Create an Organization called "timeswap-labs".
2) Create a package called "@timeswap-labs/timeswap-v1-core" under "timeswap-labs" Organization.
3) Attacker can able to upload malicious code on unclaimed npm package with a higher version like 99.99.99
4) Now If any user/timeswap developer installs it by npm install package.json. The malicious pkg will be executed.
Till now "The Package is not claimed on NPM Registry, but it's vulnerable to dependency confusion".
You can read more dependency confusion here: https://dhiyaneshgeek.github.io/web/security/2021/09/04/dependency-confusion/

Mitigation:
Claim the Scope name called "timeswap-labs" By Following the above POC Step 1.

#CODE4ARENA #MEDIUM #REPORT #NPM #TIMESWAP

(code-423n4/2022-05-sturdy-findings)
Source, Code1, Code2

Number: 18
Problem: UNISWAP_FEE is hardcoded. UNISWAP_FEE is hardcoded which will lead to significant losses compared to optimal routing
Severity: Medium

Impact:
In YieldManager, UNISWAP_FEE is hardcoded, which reduce significantly the possibilities and will lead to non optimal routes. In particular, all swaps using ETH path will use the wrong pool as it will use the ETH / USDC 1% one due to this line.

Proof of Concept:
For example for CRV / USDC, the optimal route is currently CRV -> ETH and ETH -> USDC, and the pool ETH / USDC with 1% fees is tiny compared to the ones with 0.3 or 0.1%. Therefore using the current implementation would create a significant loss of revenue.

Mitigation:
Basic mitigation would be to hardcode in advance the best Uniswap paths in a mapping like it’s done for Curve pools, then pass this path already computed to the swapping library. This would allow for complex route and save gas costs as you would avoid computing them in swapExactTokensForTokens.
Then, speaking from experience, as distributeYield is onlyAdmin, you may want to add the possibility to do the swaps through an efficient aggregator like 1Inch or Paraswap, it will be way more optimal.

#CODE4ARENA #MEDIUM #REPORT #HARDCODED #STURDYFINANCE

(code-423n4/2022-05-sturdy-findings)
Source, Code

Number: 19
Problem: Unbounded Loop
Reward Manager of the Convex Base Reward Pool Can DoS processYield()
Severity: Medium

Impact:
The ConvexCurveLPVault.sol contract allows users to earn a yield on curve token deposits. Rewards are paid out in native CRV and CVX tokens but the reward manager of the base pool may opt to add extra rewards. Because the reward manager has the ability to extend the list of extra rewards, they can extend it such that the processYield() function is unable to execute within a single block. As a result, the protocol effectively loses out on all yield accrued by user's deposits. This yield is forever locked in the contract as the yield is never transferred out from the vault contract.

Mitigation:
Consider restricting the number of extra rewards by only iterating through the first X number of tokens in processYield().

#CODE4ARENA #MEDIUM #REPORT #DOS #STURDYFINANCE

(code-423n4/2022-05-sturdy-findings)
Source, Code

Number: 20
Problem: Transfer zero amount can be reverted.
ConvexCurveLPVault's _transferYield can become stuck with zero reward transfer
Severity: Medium

Impact:
Now there are no checks for the amounts to be transferred via _transferYield and _processTreasury. As reward token list is external and an arbitrary token can end up there, in the case when such token doesn't allow for zero amount transfers, the reward retrieval can become unavailable.I.e. processYield() can be fully blocked for even an extended period, with some low probability, which cannot be controlled otherwise as pool reward token list is external.
Setting the severity to medium as reward gathering is a base functionality for the system and its availability is affected.

Proof of Concept:
_transferYield proceeds with sending the amounts to treasury and yieldManager without checking:
_transferYield()
_processTreasury()

The incentive token can be arbitrary. Some ERC20 do not allow zero amounts to be sent:
Source

In a situation of such a token added to reward list and zero incentive amount earned the whole processYield call will revert, making reward gathering unavailable until either such token be removed from pool's reward token list or some non-zero reward amount be earned. Both are external processes and aren’t controllable.

Mitigation:
Consider running the transfers in _transferYield only when yieldAmount is positive:

+ if (yieldAmount > 0) {
// transfer to treasury
if (_vaultFee > 0) {
uint256 treasuryAmount = _processTreasury(_asset, yieldAmount);
yieldAmount = yieldAmount.sub(treasuryAmount);
}

// transfer to yieldManager
address yieldManager = _addressesProvider.getAddress('YIELD_MANAGER');
TransferHelper.safeTransfer(_asset, yieldManager, yieldAmount);
+ }

#CODE4ARENA #MEDIUM #REPORT #ERC20 #STURDYFINANCE

(code-423n4/2022-05-sturdy-findings)
Source, Code

Number: 21
Problem: Div by 0. Withdraw all with amount: type(uint256).max in native token (ETH) will always revert.
Severity: Medium

Impact:
if the user set amount as type(uint256).max and _asset is set as the native token at the same time, the transaction will always fail at L122 because IERC20Detailed(_asset).decimals() will revert.

Mitigation:
Change to:
if (_amount == type(uint256).max && _asset != address(0)) {

#CODE4ARENA #MEDIUM #REPORT #ETH #STURDYFINANCE

(code-423n4/2022-04-backed-findings)
Source, Code

Number: 22
Problem: sendCollateralTo is unchecked in closeLoan(), which can cause user's collateral NFT to be frozen
Severity: Medium

Impact:
The sendCollateralTo will receive the collateral NFT when closeLoan() is called. However, if sendCollateralTo is a contract address that does not support ERC721, the collateral NFT can be frozen in the contract.

Mitigation:
Replace transferFrom with safeTransferFrom

#CODE4ARENA #MEDIUM #REPORT #ERC721 #BACKED

(code-423n4/2022-04-backed-findings)
Source, Code

Number: 23
Problem: A large denominator would be the same result with the not same input. NFTLoanFacilitator: Insufficient granularity allows for same-term loans to be accepted
Severity: Medium

Impact:
It is possible for the calculated interest rate improvement to be zero if the existing interest rate is low enough (≤ 0.9% with 10% improvement rate). In such cases, lenders can compete to continually buyout each other with the same terms. The borrower will at the very least benefit from having his loan duration reset.

Proof of concept:
Assume a loan of 0.9% interest is specified. Minimum improvement rate is unchanged at 10%.
The minimal rate improvement will be:

previousInterestRate * requiredImprovementRate / SCALAR
= 9 * 100 / 1000
= 0

Mitigation:
A common unit of interest rates is basis points. Increase SCALAR to 10_000. Additionally, have interest rates be specified at intervals of 10 = 0.1%.

require(interestRate % 10 == 0, 'NFTLoanFacilitator: interest rate too granular');

#CODE4ARENA #MEDIUM #REPORT #MATH #BACKED

(code-423n4/2022-04-backed-findings)
Source, Code

Number: 24
Problem: Use safe ERC721 mint. mintBorrowTicketTo can be a contract with no onERC721Received method, which may cause the BorrowTicket NFT to be frozen and put users' funds at risk.
Severity: Medium

Impact:
IERC721Mintable(borrowTicketContract).mint(mintBorrowTicketTo, id);

If mintBorrowTicketTo is a contract that does not implement the onERC721Received method, in the current implementation of createLoan(), the tx will still be successfully, and the loan will be created.
This can be a problem if mintBorrowTicketTo can not handle ERC721 properly, as the BorrowTicket NFT will be used later to get back the user's funds.

Mitigation:

Consider using safeMint in NFTLoanTicket.sol#mint():

function mint(address to, uint256 tokenId) external override loanFacilitatorOnly {
_safeMint(to, tokenId);
}

#CODE4ARENA #MEDIUM #REPORT #ERC721 #BACKED

(code-423n4/2022-01-timeswap-findings)
Source, Code

Number: 25
Problem: Insufficient ERC20.decimal handling. safeDecimals can revert causing DoS.
Severity: Medium

Impact: The safeDecimals() function, found in the SafeMetadata.sol contract and called in 3 different Timeswap Convenience contracts, can cause a revert. This is because the safeDecimals function attempts to use abi.decode to return a uint8 when data.length >= 32. However, a data.length value greater than 32 will cause abi.decode to revert.
A similar issue was found in a previoud code4rena contest: code-423n4/2021-05-nftx-findings#46


Proof of Concept:
The following link shows the safeDecimals() function in the BoringCrypto library, which might be where this code was borrowed from, uses the strict equality check data.length == 32

safeDecimals() is used in multiple functions such as:

-CollateralizedDebt.sol line 50 and line 54
-Bond.sol line 34
-Insurance.sol line 36

Mitigation:
Modify the safeDecimals() function to change from >= 32 to == 32 like this
if (success && data.length == 32) return abi.decode(data, (uint8));

#CODE4ARENA #MEDIUM #REPORT #ERC20 #TIMESWAP