Y2k Finance contest - Aymen0909's results

QA Report

Summary

Findings

1- Immutable state variables lack zero value -address or uint- checks :

Constructors should check the values written in an immutable state variables(address, uint, int) is not the zero value (address(0) or 0)

Impact - Low Risk

Proof of Concept

Instances include:

File: src/Vault.sol

strikePrice = _strikePrice;

File: src/rewards/StakingRewards.sol

rewardsToken = ERC20(_rewardsToken);

stakingToken = IERC1155(_stakingToken);

Mitigation

Add non-zero value -address or uint/int- checks in the constructors for the instances aforementioned.

2- require should be used instead of assert :

Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction’s available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that “The assert function creates an error of type Panic(uint256). … Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix”.

Impact - Low Risk

Proof of Concept

Instances include:

File: src/Vault.sol

assert(IWETH(address(asset)).transfer(msg.sender, msg.value));

Mitigation

Replace the assert checks aforementioned with require statements.

3- Setters should check the input value and revert if it's the zero address or zero :

Impact - Low Risk

Proof of Concept

Instances include:

File: src/VaultFactory.sol

function changeTimewindow(uint256 _marketIndex, uint256 _timewindow)

File: src/rewards/StakingRewards.sol

function setRewardsDuration(uint256 _rewardsDuration)

File: src/rewards/RewardsDistributionRecipient.sol

function setRewardsDistribution(address _rewardsDistribution)

Mitigation

Add non-zero checks for uint to the setters aforementioned.

4- Redundant ZERO address check when changing controller in VaultFactory contract :

The changeController function in the VaultFactory contract check that the new controller address is not address(0) and then calls the changeController function from the Vault contract which also checks for non-zero address which not necessary and redundant as only the factory can call the changeController function inside the Vault contract.

Impact - NON CRITICAL

Proof of Concept

Instances occurs in :

File: src/VaultFactory.sol

function changeController(uint256 _marketIndex, address _controller) public onlyAdmin

File: src/rewards/StakingRewards.sol

function changeController(address _controller) public onlyFactory

As you can see both changeController functions checks for the non-zero _controller address :

File: src/VaultFactory.sol

    function changeController(uint256 _marketIndex, address _controller)
        public
        onlyAdmin
    {
        if(_controller == address(0))
            revert AddressZero();

        address[] memory vaults = indexVaults[_marketIndex];
        Vault insr = Vault(vaults[0]);
        Vault risk = Vault(vaults[1]);
        insr.changeController(_controller);
        risk.changeController(_controller);

        emit changedController(_marketIndex, _controller);
    }

File: src/Vault.sol

    function changeController(address _controller) public onlyFactory {
        if(_controller == address(0))
            revert AddressZero();
        controller = _controller;
    }

Mitigation

Remove one of those checks and leave only the non-zero _controller address checks in the changeController function from the VaultFactory.

5- Misorganized checks in the createNewMarket function inside the VaultFactory contract :

In the createNewMarket function there is first a check that calls the controller contract to get the vault factory and then a check to verify that the controller is set (controller != address(0)), which doesn't make sense because if the controller is not set the first check will automaticaly revert as getVaultFactory() will return address(0) thus the second check is unecessary.

Impact - NON CRITICAL

Proof of Concept

Instances occurs in :

File: src/VaultFactory.sol

    if(
        IController(controller).getVaultFactory() != address(this)
        )
        revert AddressFactoryNotInController();

    if(controller == address(0))
        revert ControllerNotSet();

Mitigation

To fix this issue there is two solutions :

• Remove completely the controller == address(0) check as it's redundant in this case.

• Inverse the two checks so that the createNewMarket function checks first if the controller is set and then calls the controller contract, in this case if the controller is not set it will avoid an unecessary external call.

6- Named return variables not used anywhere in the function :

When Named return variable are declared they should be used inside the function instead of the return statement or if not they should be removed to avoid confusion.

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: src/VaultFactory.sol

returns (address insr, address rsk)

returns (address[] memory vaults)

File: src/Vault.sol

returns (uint256 shares)

returns (uint256 shares)

returns (uint256 shares)

returns (uint256 feeValue)

returns (uint256 entitledAmount)

returns (uint256 nextEpochEnd)

File: src/Controller.sol

returns (int256 nowPrice)

File: src/rewards/RewardsFactory.sol

returns (address insr, address risk)

returns (bytes32 hashedIndex)

Mitigation

Either use the named return variables inplace of the return statement or remove them.

7- public functions not called by the contrat should be declared external instead :

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: src/VaultFactory.sol

function createNewMarket(uint256 _withdrawalFee, address _token, int256 _strikePrice, uint256 epochBegin, uint256 epochEnd, address _oracle, string memory _name) public

function deployMoreAssets(uint256 index, uint256 epochBegin, uint256 epochEnd, uint256 _withdrawalFee) public

function setController(address _controller) public

function changeTreasury(address _treasury, uint256 _marketIndex) public

function changeTimewindow(uint256 _marketIndex, uint256 _timewindow) public

function changeController(uint256 _marketIndex, address _controller) public

function changeOracle(address _token, address _oracle) public

function getVaults(uint256 index) public

File: src/Vault.sol

function deposit(uint256 id, uint256 assets, address receiver) public

function changeTreasury(address _treasury) public

function changeTimewindow(uint256 _timewindow) public

function changeController(address _controller) public

function createAssets(uint256 epochBegin, uint256 epochEnd, uint256 _withdrawalFee) public

function endEpoch(uint256 id, bool depeg) public

function setClaimTVL(uint256 id, uint256 claimTVL) public

function sendTokens(uint256 id, address _counterparty) public

File: src/Controller.sol

function triggerDepeg(uint256 marketIndex, uint256 epochEnd) public

function triggerEndEpoch(uint256 marketIndex, uint256 epochEnd) public

Mitigation

Declare the functions aforementioned external.

8- Use scientific notation for better readability :

For readability, it is best to use scientific notation (e.g 10e5) rather than decimal literals(100000) or exponentiation(10**5)

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: src/oracles/PegOracle.sol

nowPrice = (price2 * 10000) / price1;

nowPrice = (price1 * 10000) / price2;

nowPrice / 1000000

File: src/Vault.sol

return (amount * epochFee[_epoch]) / 1000;

Mitigation

Replace the numbers aforementioned with their scientific notation

9- constant should be defined rather than using magic numbers :

It is best practice to use constant variables rather than hex/numeric literal values to make the code easier to understand and maintain.

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: src/Vault.sol

if(_withdrawalFee > 150)

keccak256(abi.encodePacked("rY2K"))

Mitigation

Replace the hex/numeric literals aforementioned with constants

Gas Optimizations

Summary

Findings

1- Multiple address mappings can be combined into a single mapping of an address to a struct :

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There are 3 instances of this issue :

File: src/rewards/StakingRewards.sol

43      mapping(address => uint256) public userRewardPerTokenPaid;
44      mapping(address => uint256) public rewards;
47      mapping(address => uint256) private _balances;

These mappings could be refactored into the following struct and mapping for example :

struct User {
    uint256 userRewardPerTokenPaid;
    uint256 rewards;
    uint256 _balances;
}
    
mapping(address => User) public users;

2- indexVaults mapping should use a fixed-length array to save gas :

The indexVaults mapping stores the hedge & risk vault addresses for a given market index, because there will be always only two addresses to be stored [address(hedge), address(risk)] the mapping should use a fixed-length (static) array of 2 elements address[2] instead of a dynamic array, this will save ~20000 Gas when creating a new market.

There is 1 instance of this issue :

File: src/Vault.sol

119     mapping(uint256 => address[]) public indexVaults;

This should be replaced by :

mapping(uint256 => address[2]) public indexVaults;

3- State variables only set in the constructor should be declared immutable :

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

There are 5 instances of this issue:

File: src/SemiFungibleVault.sol

20      string public name;
21      string public symbol;

File: src/rewards/RewardsFactory.sol

17      address public admin;
18      address public govToken;
19      address public factory;

4- epochsLength() should not be looked up in every iteration in the for loop :

epochsLength() reads directely from storage thus costing more gas, it shouldn't be called at each iteration, its value should be cached into a memory variable

There is 1 instance of this issue:

File: src/Vault.sol

    for (uint256 i = 0; i < epochsLength(); i++) {
        if (epochs[i] == _epoch) {
            if (i == epochsLength() - 1) {
                return 0;
            }
            return epochs[i + 1];
        }
    }

This should be refactored as follow :

    uint256 len = epochsLength();
    for (uint256 i = 0; i < len; i++) {
        if (epochs[i] == _epoch) {
            if (i == len - 1) {
                return 0;
            }
            return epochs[i + 1];
        }
    }

5- Use custom errors rather than require()/revert() strings to save deployments gas :

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information.

There are 19 instances of this issue :

File: src/Vault.sol

165     require((shares = previewDeposit(id, assets)) != 0, "ZeroValue");
187     require(msg.value > 0, "ZeroValue");

File: src/SemiFungibleVault.sol

91      require((shares = previewDeposit(id, assets)) != 0, "ZERO_SHARES");
116     require(
            msg.sender == owner || isApprovedForAll(owner, receiver),
            "Only owner can withdraw, or owner has approved receiver for all"
        );

File: src/rewards/StakingRewards.sol

96      require(amount != 0, "Cannot stake 0");
119     require(amount > 0, "Cannot withdraw 0");
202     require(
            rewardRate <= balance.div(rewardsDuration),
            "Provided reward too high"
        );
217     require(
            tokenAddress != address(stakingToken),
            "Cannot withdraw the staking token"
        );
226     require(
            block.timestamp > periodFinish,
            "Previous rewards period must be complete before changing the duration for the new period"
        );

File: src/oracles/PegOracle.sol

23      require(_oracle1 != address(0), "oracle1 cannot be the zero address");
24      require(_oracle2 != address(0), "oracle2 cannot be the zero address");
25      require(_oracle1 != _oracle2, "Cannot be same Oracle");
28      require(
            (priceFeed1.decimals() == priceFeed2.decimals()),
            "Decimals must be the same"
        );
98      require(price1 > 0, "Chainlink price <= 0");
99      require(
            answeredInRound1 >= roundID1,
            "RoundID from Oracle is outdated!"
        );
103     require(timeStamp1 != 0, "Timestamp == 0 !");
121     require(price2 > 0, "Chainlink price <= 0");
122     require(
            answeredInRound2 >= roundID2,
            "RoundID from Oracle is outdated!"
        );
126     require(timeStamp2 != 0, "Timestamp == 0 !");

6- It costs more gas to initialize variables to zero than to let the default of zero be applied :

If a variable is not set/initialized, it is assumed to have the default value (0 for uint or int, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

There are 3 instances of this issue:

File: src/rewards/StakingRewards.sol

36      uint256 public periodFinish = 0;

File: src/Vault.sol

443     for (uint256 i = 0; i < epochsLength(); i++)

File: src/VaultFactory.sol

159     marketIndex = 0;

7- Use of ++i cost less gas than i++/i=i+1 in for loops :

There is 1 instance of this issue:

File: src/Vault.sol

443     for (uint256 i = 0; i < epochsLength(); i++)

8- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There is 1 instance of this issue:

File: src/Vault.sol

443     for (uint256 i = 0; i < epochsLength(); i++)

9- Functions guaranteed to revert when called by normal users can be marked payable :

If a function modifier such as onlyAdmin is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for the owner because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are :

CALLVALUE(gas=2), DUP1(gas=3), ISZERO(gas=3), PUSH2(gas=3), JUMPI(gas=10), PUSH1(gas=3), DUP1(gas=3), REVERT(gas=0), JUMPDEST(gas=1), POP(gas=2). Which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 18 instances of this issue:

File: src/Vault.sol

227     function changeTreasury(address _treasury) public onlyFactory
287     function changeTimewindow(uint256 _timewindow) public onlyFactory
295     function changeController(address _controller) public onlyFactory
307     function createAssets(uint256 epochBegin, uint256 epochEnd, uint256 _withdrawalFee) public onlyFactory
336     function endEpoch(uint256 id, bool depeg) public onlyController marketExists(id)
350     function setClaimTVL(uint256 id, uint256 claimTVL) public onlyController
360     function sendTokens(uint256 id, address _counterparty) public onlyController marketExists(id)

File: src/VaultFactory.sol

178     function createNewMarket(uint256 _withdrawalFee, address _token, int256 _strikePrice, uint256 epochBegin, uint256 epochEnd, address _oracle, string memory _name) public onlyAdmin returns (address insr, address rsk)
248     function deployMoreAssets(uint256 index, uint256 epochBegin, uint256 epochEnd, uint256 _withdrawalFee) public onlyAdmin
295     function setController(address _controller) public onlyAdmin
308     function changeTreasury(address _treasury, uint256 _marketIndex) public onlyAdmin
327     function changeTimewindow(uint256 _marketIndex, uint256 _timewindow) public onlyAdmin
345     function changeController(uint256 _marketIndex, address _controller) public onlyAdmin
366     function changeOracle(address _token, address _oracle) public onlyAdmin

File: src/rewards/RewardsFactory.sol

83      function createStakingRewards(uint256 _marketIndex, uint256 _epochEnd, uint256 _rewardDuration, uint256 _rewardRate)
        external
        onlyAdmin
        returns (address insr, address risk)

File: src/rewards/StakingRewards.sol

213     function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyOwner
225     function setRewardsDuration(uint256 _rewardsDuration) external onlyOwner

File: src/rewards/RewardsDistributionRecipient.sol

20      function setRewardsDistribution(address _rewardsDistribution) external onlyOwner