Y2k Finance contest - jonatascm's results

createNewMarket have inverted condition check

Target codebase

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L187-L193

Vulnerability details

The function createNewMarket incorrect revert if input with zero address because of inverted order of if conditions.

Recommended Mitigation Steps

Is recommended to follow the code snippet:

function createNewMarket(...) ... {
+ if(controller == address(0))
+   revert ControllerNotSet();

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

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

  ...
}

---

Event not used

Target codebase

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L97

Vulnerability details

The event is declared but not used.

Recommended Mitigation Steps

Is recommended to either add emit to event in correct function or remove it.

---

Return is not necessary

Target codebase

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L425

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/oracles/PegOracle.sol#L76-#82

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/oracles/PegOracle.sol#L105

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/oracles/PegOracle.sol#L128

Vulnerability details

Some functions return already return named variables, this is not needed

Recommended Mitigation Steps

Is recommended to follow the code snippet:

//Function with already named return: amount	
function randomFnc(uint256 x) external returns(uint256 amount){
	amount = x * 2;
	//Is not necessary to return amount
	//return amount;
}

//In PegOracle.sol
- nowPrice = nowPrice * decimals10;
+ nowPrice = (nowPrice * decimals10) / 1000000;

- return (
-    roundID1,
-    nowPrice / 1000000,
-    startedAt1,
-    timeStamp1,
-    answeredInRound1
- );

---

Lack of zero address check

Target codebase

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L308

Vulnerability details

Some bugs happens because of invalid inputs, it's a good pattern to check if input values are valid or inside a range

Recommended Mitigation Steps

Is recommended to follow this changes:

function changeTreasury(address _treasury, uint256 _marketIndex)
    public
    onlyAdmin
{
+	if(_treasury == address(0))
+    revert AddressZero();

  treasury = _treasury;
	...
}

++i costs less gas compared to i++ or i += 1

Target codebase

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L195

Vulnerability details

++i costs less gas compared to i++ or i += 1 for unsigned integers. This is because the pre-increment operation is cheaper (about 5 GAS per iteration).

Recommended Mitigation Steps

Is recommended to follow this code snippet:

- marketIndex += 1;
+ ++marketIndex;

---

External function consumes less gas than public

Target codebase

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L438

Vulnerability details

Functions with the public visibility modifier cost more gas than external

Recommended Mitigation Steps

Is recommended to use external instead of public in functions that will be only used externally, example:

contract Test {

    string message = "Hello World";

    // Execution cost: 24527
    function test() public view returns (string memory){
         return message;
    }

    //Execution cost: 24505
    function test2() external view returns  (string memory){
         return message;
    }
}

---

Optimize loops

Target codebase

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L443

Vulnerability details

The loops aren't optimized.

Recommended Mitigation Steps

1. Default values don't need to be declared

   - uint256 i = 0;
   - bool flag = false;
   
   + uint256 i;
   + bool flag;

2. Cache array length

   - for (...; i < epochsLength(); ...) {
   
   + uint256 eLength = epochsLength();
   + for (...; i < eLength; ...) {

3. Uncheck loop and use ++i

   - for (...; ...; i++) {
   + for (...; ...;) {
   	...
   + unchecked{ ++i; }
   }

Follow this code to save gas:

- for (uint256 i = 0; i < epochsLength(); i++) {

+ uint256 eLength = epochsLength();
+ for (uint256 i; i < eLength;) {
	...
+ unchecked{ ++i; }
}

---

Default values don't need to be declared

Target codebase

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/rewards/StakingRewards.sol#L36

Vulnerability details

Declaring default values for variables cost more gas.

Recommended Mitigation Steps

Follow this code snippet:

- uint256 i = 0;
- bool flag = false;

+ uint256 i;
+ bool flag;