(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

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

Number: 26
Problem: no reentrancy guard on mint() function that has a callback
Severity: Medium

Impact: In CollateralizedDebt.sol, the mint() function calls _safeMint() which has a callback to the "to" address argument. Functions with callbacks should have reentrancy guards in place for protection against possible malicious actors both from inside and outside the protocol.

Proof of Concept:
Code1
Code2

Mitigation:
Add a reentrancy guard modifier on the mint() function in CollateralizedDebt.sol

#CODE4ARENA #MEDIUM #REPORT #REENTRANCY #ERC721 #TIMESWAP

Project: Harpie
Platform: Sherlock
Source, Code

Number: 27
Problem: Use safeTransferFrom() instead of transferFrom() for outgoing erc721 transfers
Severity: Medium

Vulnerability detail:
The transferFrom() method is used instead of safeTransferFrom(), which I assume is a gas-saving measure. I however argue that this isn’t recommended because:

OpenZeppelin’s documentation discourages the use of transferFrom(); use safeTransferFrom() whenever possible
The recipient could have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom().

Impact:
While unlikely because the recipient is the function caller, there is the potential loss of NFTs should the recipient is unable to handle the sent ERC721s.

Recommendation:
Use safeTransferFrom() when sending out the NFT from the vault.
- IERC721(_erc721Address).transferFrom(address(this), msg.sender, _id);
+ IERC721(_erc721Address).safeTransferFrom(address(this), msg.sender, _id);

Fix

#SHERLOCK #MEDIUM #REPORT #ERC721 #HARPIE

Project: Harpie
Platform: Sherlock
Source, Code

Number: 28
Problem: Cross-chain replay attacks are possible with changeRecipientAddress()
Severity: Medium

Vulnerability detail:
Mistakes made on one chain can be re-applied to a new chain
There is no chain.id in the signed data

Impact:
If a user does a changeRecipientAddress() using the wrong network, an attacker can replay the action on the correct chain, and steal the funds a-la the wintermute gnosis safe attack, where the attacker can create the same address that the user tried to, and steal the funds from there.

Recommendation:
Include the chain.id in what's hashed

Fix

#SHERLOCK #MEDIUM #REPORT #SIGNATURE #HARPIE

Project: Harpie
Platform: Sherlock
Source, Code

Number: 29
Problem: Incompatability with deflationary / fee-on-transfer tokens
Severity: Medium

Vulnerability detail:
In case ERC20 token is fee-on-transfer, Vault can loss funds when users withdraw
In Transfer.transferERC20() function, this function is called logIncomingERC20() with the exact amount used when it is called safeTransferFrom(). In case ERC20 token is fee-on-transfer, the actual amount that Vault received may be less than the amount is recorded in logIncomingERC20().

Impact:
The result is when a user withdraws his funds from Vault, Vault can be lost and it may make it unable for later users to withdraw their funds.

Proof of Concept:
-Token X is fee-on-transfer and it took 10% for each transfer. Alice has 1000 token X and Bob has 2000 token X
-Assume that both Alice and Bob are attacked. Harpie transfers all token of Alice and Bob to Vault. It recorded that the amount stored for token X of Alice is 1000 and Bob is 2000. But since token X has 10% fee, Vault only receives 2700 token X.
-Now Bob withdraw his funds back. With amountStored = 2000, he will transfer 2000 token X out of the Vault and received 1800.
-Now the Vault only has 700 token X left and obviously it's unable for Alice to withdraw

Recommendation:
Consider calculating the actual amount Vault received to call logIncomingERC20()
Transfer the tokens first and compare pre-/after token balances to compute the actual transferred amount.

Lead Senior Watson:
While it's true the fix does allow for compatabiliy with fee-on-transfer tokens, it does not correctly handle rebasing tokens. Might be useful to explicily note that rebasing tokens are not supported or instead you could adopt mint shares to represent the ownership over the vault's tokens.

Harpie:
On rebasing tokens, we just won't be able to support them for now.

Fix

#SHERLOCK #MEDIUM #REPORT #DEFLATIONARY #HARPIE

Project: Harpie
Platform: Sherlock
Source, Code

Number: 30
Problem: Usage of deprecated transfer() can result in revert.
Severity: Medium

Vulnerability detail:
transfer() uses a fixed amount of gas, which was used to prevent reentrancy. However this limit your protocol to interact with others contracts that need more than that to process the transaction.

Specifically, the withdrawal will inevitably fail when:
1. The withdrawer smart contract does not implement a payable fallback function.
2. The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
3. The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

Impact:
transfer() uses a fixed amount of gas, which can result in revert.
https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Recommendation:
Use call instead of transfer(). Example:
(bool succeeded, ) = _to.call{value: _amount}("");
require(succeeded, "Transfer failed.");

Fix

#SHERLOCK #MEDIUM #REPORT #TRANSFER #HARPIE

Project: Harpie
Platform: Sherlock
Source, transferERC20, transferERC721

Number: 31
Problem: There is no limit on the amount of fee users have to pay
Severity: Medium

Vulnerability detail:
There is no upper limit on the amount of fee users have to pay to withdraw their funds back. So any EOA can call transfer function on Transfer contract can set an unreasonable amount of fee and users have to pay it if they want their funds back. We need to make sure that users' funds cannot be loss even when the protocol acts maliciously.

Impact:
In case the protocol acts maliciously and set fee = 1e18 to transfer users' fund to Vault, users cannot withdraw their funds since fee is too high.

Proof of Concept:
In both transferERC20() and transferERC721(), EOA is the caller and can set the fee param to any value it wants.
And users need to send enough fee (native token) to withdraw their fund back on Vault.
Code

Recommendation:
Consider adding an upper limit on the amount of fee users need to pay.
Fix
Additional fixes:
- Fix
- Fix
- Fix

#SHERLOCK #MEDIUM #REPORT #FEE #HARPIE

Project: Harpie
Platform: Sherlock
Source, Code

Number: 33
Problem: Unsafe casting of user amount from uint256 to uint128
Severity: Medium

Vulnerability detail:
The unsafe casting of the recovered amount from uint256 to uint128 means the users’ funds will be lost.
logIncomingERC20() has the recovered amount as type uint256, but amountStored is of type uint128. There is an unsafe casting when incrementing amountStored:

_erc20WithdrawalAllowances[_originalAddress][_erc20Address].amountStored += uint128(_amount);

It is thus possible for the amount recorded to be less than the actual amount recovered.

Impact:
Loss of funds

Proof of Concept:
The user's balance is type(uint128).max = 2**128, but the incremented amount will be zero.

Recommendation:
amountStored should be of type uint256. Alternatively, use OpenZeppelin’s SafeCast library when casting from uint256 to uint128.

Lead Senior Watson:
Not sure, any tokens which would have a token supply over type(uint128).max but I guess it's best to be proactive. The proposed fix does create some issues. Instead of having less tokens transferred to the vault, the contract will revert and prevent the transfer entirely. Arguably more funds would be at risk, so you may as well use uint256 then or accept the risk and keep the slot packing.

Harpie Team:
Decided to accept the risk of reverts since it's a lot of gas savings and there probably arent useful tokens with supply over (uint128).max. Used @openzeppelin/SafeCast

Fix

#SHERLOCK #MEDIUM #REPORT #CAST #HARPIE

Project: Harpie
Platform: Sherlock
Source, Code

Number: 34
Problem: reduceERC721Fee function can not set fee when the NFT token ID is more than type(uint128).max
Severity: Medium

Vulnerability detail:
reduceERC721Fee function can not set fee when the NFT token ID is more than type(uint128).max. The NFT token ID can be any value within uint 256.
As the reduceERC721Fee takes the _id argument as uint 128, when the reduceERC721Fee function is called with an NFT id that has above type(uint128).max , the fee can not set to the expected NFT id.

Impact:
ERC721Fee can not set fee when the NFT token ID value is more than type(uint128).max

Recommendation:
Change the function argument for reduceERC721Fee as shown below.
before fix : function reduceERC721Fee(address _originalAddress, address _erc721Address, uint128 _id, uint128 _reduceBy) external returns (uint128)

after fix: function reduceERC721Fee(address _originalAddress, address _erc721Address, uint256 _id, uint128 _reduceBy) external returns (uint128)

Lead Senior Watson:
ERC721 standard doesn't enforce how tokenId is implemented for a given NFT. Could definitely be greater than uint128, although I've never seen a case where this is true.

Fix

#SHERLOCK #MEDIUM #REPORT #ERC721 #HARPIE

Project: Harpie
Platform: Sherlock
Source, Code

Number: 35
Problem: Nonces not used in signed data
Severity: Medium

Vulnerability detail:
Nonces are not used in the signature checks. A nonce can prevent an old value from being used when a new value exists. Without one, two transactions submitted in one order, can appear in a block in a different order

Impact:
If a user is attacked, then tries to change the recipient address to a more secure address, initially chooses an insecure compromised one, but immediately notices the problem, then re-submits as a different, uncompromised address, a malicious miner can change the order of the transactions, so the insecure one is the one that ends up taking effect, letting the attacker transfer the funds

Recommendation:
Include a nonce in what is signed

Lead Senior Watson: Not an issue AFAIK, miners can't reorder txs unless they are signed with the same nonce. There would have to be some serious mis-use of this function by the recipient address, i.e. they would have to ask the server to sign for two different addresses and then broadcast the txs with the same nonce for this call. The proposed fix could probably be safely removed but doesn't hurt to keep it there.

Fix

#SHERLOCK #MEDIUM #REPORT #SIGNATURE #HARPIE

Project: Sentiment
Platform: Sherlock
Source, Code

Number: 36
Problem: A malicious early user/attacker can manipulate the LToken's pricePerShare to take an unfair share of future users' deposits
Severity: High

Vulnerability detail:
A well known attack vector for almost all shares based liquidity pool contracts, where an early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.

A malicious early user can deposit() with 1 wei of asset token as the first depositor of the LToken, and get 1 wei of shares. Then the attacker can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) .
As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token. They will immediately lose 9999e18 or half of their deposits if they redeem() right after the deposit().

Impact:
The attacker can profit from future users' deposits. While the late users will lose part of their funds to the attacker.

Recommendation:
Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a port of the initial mints as a reserve to the DAO so that the pricePerShare can be more resistant to manipulation.

Fix

#SHERLOCK #HIGH #REPORT #SHARES #SENTIMENT

https://cointelegraph.com/news/how-to-avoid-front-runners-on-decentralized-crypto-exchanges

https://consensys.github.io/smart-contract-best-practices/attacks/frontrunning/


Optimal frontrunning attacks and how to stop them
https://youtu.be/BwkNQWM32y0

#EDUCATION