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

https://www.paradigm.xyz/2023/01/eth-rng

https://soliditydeveloper.com/prevrandao

@ethers_officer

🔴Many security vulnerabilities come from faulty assumptions

Identifying the assumptions made by the devs and evaluating if they are correct can uncover big discrepancies between what the code does vs what it is intended to do

Here are examples of common faulty assumptions: 📔

1. Initialization functions will only be called ONCE and/or can be called only by the contract deployer

2. Only admins can call certain functions(access control issues)

3. Functions will always be called in a certain order as expected by the system

Ex. what if there's a function that closes a position but expects that you opened one in the 1st place?

A function that checks if your payment is on time but expects you got a loan before that?

4. Parameters can only have non-zero values or values within a certain threshold

addresses will never be zero-valued
sender will always be different from the receiver
an element of a struct array will always exist so the values won't be the default ones

5. Certain addresses or data values can never be attacker-controlled

6. Function calls will always be successful and so checking for return values is not required
These are just a few examples of common assumptions that don't always hold true
Always try to identify what assumptions are made when writing the code and compare that to how the system could actually behave
@EthSecurity1

nonReentrant modifiers might potentially cause a DoS attack.
https://medium.com/@bloqarl/uncovering-real-life-examples-of-denial-of-service-attacks-on-smart-contracts-8bc220c2cdd0
@EthSecurity1