Y2k Finance contest - 0x1f8b's results

Judge has assessed an item in Issue #62 as Medium risk. The relevant finding follows:

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/oracles/PegOracle.sol#L63 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Controller.sol#L308 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/oracles/PegOracle.sol#L126

Vulnerability details

Impact

Different problems have been found with the use of the oracle that can incur economic losses when the oracle is not consumed in a completely safe way.

Proof of Concept

Thre problems found are:

• The timeStamp check is not correct since in both cases it is done against 0, which would mean that a date of 2 years ago would be valid, so old prices can be taken.

    function getLatestPrice(address _token)
        public
        view
        returns (int256 nowPrice)
    {
        ...
        if(timeStamp == 0)
            revert TimestampZero();
        return price;
    }

• Oracle price 1 can be outdated:

The latestRoundData method of the PegOracle contract calls priceFeed1.latestRoundData(); directly, but does not perform the necessary round or timestamp checks, and delegates them to the caller, but these checks are performed on price2 because it calls getOracle2_Price in this case, this inconsistency between how it take the price1 and price2 behaves favors human errors when consuming the oracle.

Recommended Mitigation Steps

For the timestamp issue, it should be checked like this:

+   uint constant observationFrequency = 1 hours;

    function getLatestPrice(address _token)
        public
        view
        returns (int256 nowPrice)
    {
        ...
        (
            uint80 roundID,
            int256 price,
            ,
            uint256 timeStamp,
            uint80 answeredInRound
        ) = priceFeed.latestRoundData();

        uint256 decimals = 10**(18-(priceFeed.decimals()));
        price = price * int256(decimals);

        if(price <= 0)
            revert OraclePriceZero();

        if(answeredInRound < roundID)
            revert RoundIDOutdated();

-       if(timeStamp == 0)
+       if(timeStamp < block.timestamp - uint256(observationFrequency))
            revert TimestampZero();

        return price;
    }

• Low
  • 1. Denial of service with tokens with > 18 decimals
  • 2. Lack of checks
  • 3. Uninsured Rewards
  • 4. Revert if the data is incorrect
  • 5. Lack of event emit
• Non critical
  • 6. Outdated compiler
  • 7. Invert conditions
  • 8. Reuse code to avoid errors
  • 9. Modifier should not modify states
  • 10. Unused method which may incur inheritance issues

Low

1. Denial of service with tokens with > 18 decimals

PegOracle contract assume decimals <= 18 and does not handle > 18 decimals.

Because the pragma used doesn't allow integer underflows, if a token with more than 18 decimals is used, an integer underflow will produce a denial of service.

Some tokens have more than 18 decimals (e.g. YAM-V2 has 24).

This may trigger unexpected reverts due to overflow, posing a liveness risk to the contract.

Reference:

Major severity finding from Consensys Diligence Audit of Defi Saver:

• https://consensys.net/diligence/audits/2021/03/defi-saver/#tokens-with-more-than-18-decimal-points-will-cause-issues

Affected source code:

• PegOracle.sol:36
• PegOracle.sol:73

2. Lack of checks

The following methods have a lack of checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code for address(0):

• RewardsFactory.sol:64-66
• StakingRewards.sol:74-76
• SemiFungibleVault.sol:70

Affected source code for integer range checks:

• StakingRewards.sol:77-79

_withdrawalFee must be less than the factor:

• VaultFactory.sol:179
• VaultFactory.sol:252

3. Uninsured Rewards

Nothing ensures that there are rewards for the users stakes, it can also be stolen from the rewardsToken by the admin.

Affected source code:

• StakingRewards.sol:136

4. Revert if the data is incorrect

The admin might think that the change has been applied and it is not. If it shouldn't call changeOracle and maybe due to human error, the admin won't.

        if (tokenToOracle[_token] == address(0)) {
            tokenToOracle[_token] = _oracle;
        }
+       else if (tokenToOracle[_token] != _oracle) {
+           revert();
+       }

Affected source code:

• VaultFactory.sol:223

5. Lack of event emit

The setClaimTVL, changeTreasury, changeTimewindow and changeController methods do not emit an event when the state changes, something that it's very important for dApps and users.

Affected source code:

• Vault.sol:277-299
• Vault.sol:351

Non critical

6. Outdated compiler

The pragma version used are:

pragma solidity 0.8.15;

Note that mixing pragma is not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.16; otherwise, contracts will be affected by the following important bug fixes:

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

Apart from these, there are several minor bug fixes and improvements.

7. Invert conditions

To make it easier to detect the error, these conditions should be reversed, otherwise the user will receive a different error.

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

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

Affected source code:

• VaultFactory.sol:187-193

8. Reuse code to avoid errors

The RewardsFactory contract replicates the logic of the getHashedIndex method in some places, it is convenient to call the method since if said calculation is updated, it will have to be changed in several places with the possible human error that this entails.

Affected source code:

Call getHashedIndex instead replicate the logic in:

• RewardsFactory.sol#L118

9. Modifier should not modify states

It is not recommended that a modifier modify the states of the contract, normally they can be called multiple times by chaining different calls, and it also complicates the readability and auditability of the code, so it is recommended to use any modifier only for validations.

Affected source code:

• StakingRewards.sol:60

10. Unused method which may incur inheritance issues

In SemiFungibleVault contract, the methods maxDeposit, maxMint and maxWithdraw are not used, if someone understands the contract and modifies these virtual methods, it will not be applied unless they also modify the deposit and withdraw methods.

Affected source code:

• SemiFungibleVault.sol:237-258

• Gas
  • 1. Reduce the size of error messages Long revert Strings
    • Total gas saved: 18 * 7 = 126
  • 2. Use Custom Errors instead of Revert Strings to save Gas
    • Total gas saved: 9 * 19 = 171
  • 3. Gas saving using immutable
  • 4. Remove unnecessary variables
  • 5. Reduce boolean comparison
    • Total gas saved: 15 * 5 = 75
    • Total gas saved: 18 * 1 = 18
    • Total gas saved: 18 * 1 = 18
  • 6. Local variable not needed for conditional
  • 7. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 1 = 13
  • 8. ++i costs less gas compared to i++ or i += 1
  • 9. Remove empty blocks
  • 10. There's no need to set default values for variables
    • Total gas saved: 8 * 3 = 24
  • 11. Don't use the method's output for loops condition
  • 12. Use require instead of assert
  • 13. Use immutable variable for optimize

Gas

1. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testShortRevert(bool path) public view {
require(path, "test error");
}
}

contract TesterB {
function testLongRevert(bool path) public view {
require(path, "test big error message, more than 32 bytes");
}
}

Gas saving executing: 18 per entry

TesterA.testShortRevert: 21886   
TesterB.testLongRevert:  21904       

Affected source code:

• SemiFungibleVault.sol:116-119
• PegOracle.sol:23
• PegOracle.sol:24
• PegOracle.sol:99-102
• PegOracle.sol:122-125
• StakingRewards.sol:217-220
• StakingRewards.sol:226-229

Total gas saved: 18 * 7 = 126

2. Use Custom Errors instead of Revert Strings to save 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)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testRevert(bool path) public view {
 require(path, "test error");
}
}

contract TesterB {
error MyError(string msg);
function testError(bool path) public view {
 if(path) revert MyError("test error");
}
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611
TesterB.testError:  21602     

Affected source code:

• SemiFungibleVault.sol:91
• SemiFungibleVault.sol:116-119
• Vault.sol:165
• Vault.sol:187
• PegOracle.sol:23
• PegOracle.sol:24
• PegOracle.sol:25
• PegOracle.sol:28-31
• PegOracle.sol:98
• PegOracle.sol:99-102
• PegOracle.sol:103
• PegOracle.sol:121
• PegOracle.sol:122-125
• PegOracle.sol:126
• StakingRewards.sol:96
• StakingRewards.sol:119
• StakingRewards.sol:202-205
• StakingRewards.sol:217-220
• StakingRewards.sol:226-229

Total gas saved: 9 * 19 = 171

3. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• PegOracle.sol:13
• PegOracle.sol:15
• PegOracle.sol:16

4. Remove unnecessary variables

The following state variables can be removed without affecting the logic of the contract since they are not used and/or are redundant.

Affected source code:

• PegOracle.sol:10
• PegOracle.sol:11

5. Reduce boolean comparison

Compare a boolean value using == true or == false instead of using the boolean value is more expensive.

NOT opcode it's cheaper than using EQUAL or NOTEQUAL when the value it's false, or just the value without == true when it's true, because it will use less opcodes inside the VM.

Proof of concept (without optimizations):

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == true; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return a; }
}

Gas saving executing: 18 per entry for == true

TesterA.testEqual:   21814
TesterB.testNot:     21796   

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == false; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return !a; }
}

Gas saving executing: 15 per entry for == false

TesterA.testEqual:   21814
TesterB.testNot:     21799

Affected source code:

Use the value instead of == false:

• RewardsFactory.sol:96
• Controller.sol:93
• Controller.sol:211
• Vault.sol:96
• Vault.sol:216-217

Total gas saved: 15 * 5 = 75

Use the value instead of != true:

• Vault.sol:80

Total gas saved: 18 * 1 = 18

Use the value instead of == true:

• Vault.sol:314

Total gas saved: 18 * 1 = 18

6. Local variable not needed for conditional

The following local variables are not necessary and can be avoided by directly using the variable value in the conditional, this will avoid creating variables on the stack and reduce computational cost.

Remove bool isSequencerUp:

• Controller.sol:277

Remove uint256 timeSinceUp:

• Controller.sol:283

7. Avoid compound assignment operator in state variables

Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint private _a;
function testShort() public {
_a += 1;
}
}

contract TesterB {
uint private _a;
function testLong() public {
_a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• VaultFactory.sol:195

Total gas saved: 13 * 1 = 13

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

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Keep in mind that this change can only be made when we are not interested in the value returned by the operation, since the result is different, you only have to apply it when you only want to increase a counter.

Affected source code:

• Vault.sol:443

9. Remove empty blocks

An empty block or an empty method generally indicates a lack of logic that it’s unnecessary and should be eliminated, call a method that literally does nothing only consumes gas unnecessarily, if it is a virtual method which is expects it to be filled by the class that implements it, it is better to use abstract contracts with just the definition.

Sample code:

pragma solidity >=0.4.0 <0.7.0;

abstract contract Feline {
    function utterance() public virtual returns (bytes32);
}

Reference:

• https://docs.soliditylang.org/en/v0.6.0/contracts.html?highlight=abstract#abstract-contracts

Affected source code:

• SemiFungibleVault.sol:276-280
• SemiFungibleVault.sol:282-286

10. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384

Affected source code:

• VaultFactory.sol:159
• StakingRewards.sol:36
• Vault.sol:443

Total gas saved: 8 * 3 = 24

11. Don't use the method's output for loops condition

It's cheaper to store the dynamic value inside a local variable and iterate over it.

Affected source code:

• Vault.sol:443

12. Use require instead of assert

The assert() and require() functions are a part of the error handling aspect in Solidity. Solidity makes use of state-reverting error handling exceptions. This means all changes made to the contract on that call or any sub-calls are undone if an error is thrown. It also flags an error.

They are quite similar as both check for conditions and if they are not met, would throw an error.

The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.

Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.

Affected source code:

• Vault.sol:190

13. Use immutable variable for optimize

In the constructor, store a new immutable variable isrY2K if the symbol is rY2K, this will avoid expensive operations like the following:

    function beforeWithdraw(uint256 id, uint256 amount)
        public
        view
        returns (uint256 entitledAmount)
    {
        // in case the risk wins aka no depeg event
        // risk users can withdraw the hedge (that is paid by the hedge buyers) and risk; withdraw = (risk + hedge)
        // hedge pay for each hedge seller = ( risk / tvl before the hedge payouts ) * tvl in hedge pool
        // in case there is a depeg event, the risk users can only withdraw the hedge
        if (
-           keccak256(abi.encodePacked(symbol)) ==
-           keccak256(abi.encodePacked("rY2K"))
+           isrY2K
        ) {
            if (!idDepegged[id]) {
                //depeg event did not happen
                /*
                entitledAmount =
                    (amount / idFinalTVL[id]) *
                    idClaimTVL[id] +
                    amount;
                */
                entitledAmount =
                    amount.divWadDown(idFinalTVL[id]).mulDivDown(
                        idClaimTVL[id],
                        1 ether
                    ) +
                    amount;
            } else {
                //depeg event did happen
                entitledAmount = amount.divWadDown(idFinalTVL[id]).mulDivDown(
                    idClaimTVL[id],
                    1 ether
                );
            }
        }

Affected source code:

• Vault.sol:388-389