Skip to content

Latest commit

 

History

History
161 lines (147 loc) · 12.4 KB

descharre-Q.md

File metadata and controls

161 lines (147 loc) · 12.4 KB
Raw

Summary

Low Risk

ID Finding Instances
L-01 Missing 0 checks in constructor 4
L-02 Unspecific compiler version pragma -
L-03 Not every year equals 365 days 1
L-04 Not enough checks on withdraw and deposit function 2

Non critical

ID Finding Instances
N-01 Use a newer solidity version -
N-02 Use a modifier instead of a function for prerequisites 5
N-03 Event is missing indexed fields 7
N-04 Make consistent use of comments in the contracts -
N-05 Use uint256 instead of uint 1
N-06 Use scientific notation (1E18) rather than exponentation (10**18) 2
N-07 Unused named return variables 11
N-08 Formatting is not always correct 3
N-09 Wrong comments 1
N-10 Constant values such as a call to keccak256(), should be immutable rather than constant 2
N-11 Only use assert when you check code that never should be false 3
N-12 Functions should be grouped according to their visibility and ordered 1

Details

Low Risk

[L-01] Missing 0 checks in constructor

When you set addresses, it's a best practice to check for 0 addresses. This way a variable can't be accidently set to the 0 address. Roles are set in the constructor of ReaperVaultV2, without a check, ADMIN can be the 0 address.

[L-02] Unspecific compiler version pragma

Avoid floating or unspecific pragmas for non-library contracts. Contracts should be deployed with the same compiler version that they have been tested with. When you lock the pragma, it ensures that the contract does not get accidently deployed with a compiler that has bugs.

This applies to the contracts in Ethos-Vault

[L-03] Not every year equals 365 days

Due to leap years, not every year equals 365 days.

[L-04] Not enough checks on withdraw function

Although _withdraw is always called with msg.sender as owner and receiver, there are no require statements to check if this is true. To be safe, it's best to add a require statement in _withdraw to check if msg.sender equals owner and receiver. One change in the withdraw function where for example owner is set as a parameter and withdraw can be exploited.

ReaperVaultV2.sol#L359-L412

The same applies to the deposit function

ReaperVaultV2.sol#L319-L338

Non critical

[N-01] Use a newer solidity version

Although it will probably be a lot of refactoring work. It's better to use newer solidity versions. Old versions might contain bugs. This applies to the contract in Ethos-Core

[N-02] Use a modifier instead of a function for prerequisites

It's more readable if you use a modifier instead of the functions for prerequisites. It's also possible to use multiple modifiers in one function and have modifiers with parameters so it's entirely possible.

ActivePool.sol
L159:
-   function getCollateral(address _collateral) external view override returns (uint) {
+   function getCollateral(address _collateral) external view override _requireValidCollateralAddress(_collateral) returns (uint) {
-       _requireValidCollateralAddress(_collateral);
        return collAmount[_collateral];
    }

L313:
-   function _requireValidCollateralAddress(address _collateral) internal view {
+   modifier _requireValidCollateralAddress(address _collateral){
        require(
            ICollateralConfig(collateralConfigAddress).isCollateralAllowed(_collateral),
            "Invalid collateral address"
        );
+       _;
    }

[N-03] Event is missing indexed fields

When an event is missing indexed fields it can be hard for off-chain scripts to efficiently filter those events.

[N-04] Make consistent use of comments in the contracts

There are only a few functions that have comments. It is a good practice to use natspec comments. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

This is used sometimes in ethos-vault but not in ethos-core. It's best to be consistent with comments and use natspec in every contract.

[N-05] Use uint256 instead of uint

For code readability it's better to make consistent use of uint256. Now both uint and uint256 are used and can lead to confusion.

[N-06] Use scientific notation (1E18) rather than exponentation (10**18)

Scientific notation should be used for better code readability.

[N-07] Unused named return variables

It's a good practice to not specify named return variables when they are never used. Instead of using named return variables, you can use natspec to keep readability.

[N-08] Formatting is not always correct

The formatting is not consistent across the different contracts. A few examples:

    for(uint256 i = 0; i < numCollaterals; i++) {

One click on format document should fix this

[N-09] Wrong comments

The comment says it returns a uint256 with 18 decimals. However the function uses the decimal function which returns the correct amount of decimals of a token. Not every token has 18 decimals, BTC is used in the tests and only has 8 decimals. So the function getPricePerFullShare() returns a uint256 with 8 decimals during testing. ReaperVaultV2.sol#L293

    /**
     * @dev Function for various UIs to display the current value of one of our yield tokens.
     * Returns an uint256 with 18 decimals of how much underlying asset one vault share represents.
     */
    function getPricePerFullShare() public view returns (uint256) {
        return totalSupply() == 0 ? 10**decimals() : (_freeFunds() * 10**decimals()) / totalSupply();
    }

[N-10] Constant values such as a call to keccak256(), should be immutable rather than constant

Constants are supposed to be used for literal values written into the code, and immutable variables should be used for expressions or values calculated in the constructor. While it doesn't save any gas, they should be used in their appropriate context.

[N-11] Only use assert when you check code that never should be false

Require is used to validate inputs and conditions before execution. assert is used to check for code that should never be false. Failing assertion probably means that there is a bug. This will also save gas if the statement fails, assert uses up all the remaining gas. Require refund the remaining gas.

Some assert statements where the validation can be false without a bug and should be changed to require

[N-12] Functions should be grouped according to their visibility and ordered

According to the solidity style guide functions should be ordered like the following:

  • constructor
  • receive function (if exists)
  • fallback function (if exists)
  • external
  • public
  • internal
  • private