search

code-423n4 / 2022-05-sturdy-findings

7 stars 3 forks source link

QA Report #77

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Low Risk Issues

[L-01] In GeneralVault.sol, ETH address is misleading

In the GeneralVault contract there is a constant, ETH, set to a magic value: https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/GeneralVault.sol#L47

This is deceiving because no-one is actually using that constant, and in the concrete vaults that deal with eth (like the LIDO one), address(0) is used to represent user making deposits in eth: https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L86

Thus we suggest removing that constant because is useless and possibly deceiving for a reader

Non-critical Issues

[N-01] Use of saveApprove() deprecated function

The usage of OZ safeApprove() is deprecated as described in the latest OZ implementation: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bfff03c0d2a59bcd8e2ead1da9aed9edf0080d05/contracts/token/ERC20/utils/SafeERC20.sol#L38-L45

It’s currently used 3 times throughout the codebase, and it should be replaced with safeIncreaseAllowance() and safeDecreaseAllowance()

HickupHH3 commented 2 years ago

Both are low issues.

iris112 commented 2 years ago

L-01, We are using the ETH constant for uniswap.

HickupHH3 commented 2 years ago

Yes, that's not the issue. What is, is the inconsistency of ETH representation. I suggest using the same ETH constant in the LIDO vault to represent ETH deposits instead of the the zero address