yAxis contest - hrkrshnn's results

Handle

hrkrshnn

Vulnerability details

The sqrt function can overflow execute invalid operation

The function sqrt is incorrect for the x = type(uint).max.

function sqrt(
    uint256 x
)
    private
    pure
    returns (uint256 y)
{
    uint256 z = (x + 1) / 2;
    y = x;
    while (z < y) {
        y = z;
        z = (x / z + z) / 2;
    }
    y = y * (10 ** 9);
}  

Because of the overflow in x + 1, the value of z is 0. The expression z = (x / z + z) / 2; in the for loop does a division by zero (an invalid opcode) for solidity versions below 0.8.0, consuming all the remaining gas in the context. Although the function ends in a halting state for x = type(uint256).max, the sqrt is well defined.

Note that the sqrt function does a final scaling, I've ignored the scaling part. It is also recommended to rename the function to sqrtAndScale or something more readable.

Handle

hrkrshnn

Vulnerability details

Redundant notHalted modifier in depositMultiple

Since the call to deposit already has the notHalted modifier, doing the check during depositMultiple is redundant. Saves at least 100 gas.

https://github.com/code-423n4/2021-09-yaxis/blob/main/contracts/v3/Vault.sol#L194

Handle

hrkrshnn

Vulnerability details

Consider making some constants as non-public to save gas

Each function part of contract's external interface is part of the function dispatch, i.e., every time a contract is called, it goes through a switch statement (a set of eq ... JUMPI blocks in EVM) matching the selector of each externally available functions with the chosen function selector (the first 4 bytes of calldata).

This means that any unnecessary function that is part of contract's external interface will lead to more gas for (almost) every single function calls to the contract. There are several cases where constants were made public. This is unnecessary; the constants can simply be read from the verified contract, i.e., it is unnecessary to expose it with a public function.

Examples

./contracts/v3/Harvester.sol:24:    uint256 public constant ONE_HUNDRED_PERCENT = 10000;
./contracts/v3/Vault.sol:29:    uint256 public constant MAX = 10000;
./contracts/v3/controllers/LegacyController.sol:20:    uint256 public constant MAX = 10000;
./contracts/v3/strategies/BaseStrategy.sol:37:    uint256 public constant ONE_HUNDRED_PERCENT = 10000;
./contracts/v3/Manager.sol:26:    uint256 public constant PENDING_STRATEGIST_TIMELOCK = 7 days;
./contracts/v3/Manager.sol:27:    uint256 public constant MAX_TOKENS = 256;

Handle

hrkrshnn

Vulnerability details

Caching the length in for loops

Consider a generic example of an array arr and the following loop:

for (uint i = 0; i < arr.length; i++) {
    // do something that doesn't change arr.length
}

In the above case, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).

This extra costs can be avoided by caching the array length (in stack):

uint length = arr.length;
for (uint i = 0; i < length; i++) {
    // do something that doesn't change arr.length
}

In the above example, the sload or mload operation is only done once and subsequently replaced by a cheap dupN instruction.

This optimization is especially important if it is a storage array or if it is a lengthy for loop.

Note that the Yul based optimizer (not enabled by default; only relevant if you are using --experimental-via-ir or the equivalent in standard JSON) can sometimes do this caching automatically. However, this is likely not the case in your project.

Examples

./contracts/v3/Vault.sol:199:        for (uint8 i; i < _amounts.length; i++) {
./contracts/v3/Vault.sol:299:        for (uint8 i; i < _tokens.length; i++) {
./contracts/v3/VaultHelper.sol:64:        for (uint8 i = 0; i < _amounts.length; i++) {
./contracts/v3/controllers/Controller.sol:458:        for (uint i = 0; i < _strategies.length; i++) {

Handle

hrkrshnn

Vulnerability details

Upgrade to at least 0.8.4

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks for free!

The advantages of versions 0.8.* over <0.8.0 are:

• Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
• Low level inliner from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
• Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
• Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Handle

hrkrshnn

Vulnerability details

Overflow in depositMultiple can lead to infinite loop and incorrect deposit

See depositMultiple function.

function depositMultiple(
    address[] calldata _tokens,
    uint256[] calldata _amounts
)
    external
    override
    notHalted
    returns (uint256 _shares)
{
    require(_tokens.length == _amounts.length, "!length");

    for (uint8 i; i < _amounts.length; i++) {
        _shares = _shares.add(deposit(_tokens[i], _amounts[i]));
    }
}

There are no checks that _amounts.length < 255 = type(uint8).max. This means that the following (hypothetical) situation is possible:

1. User wants to deposit the same token A 256 number of times.
2. User calls depositMultiple with _tokens = [address(A), address(A), ..., Address(A)] and _amounts = [// some numbers].
3. After i = 255, since i is of type uint8 and type(uint8).max = 255, i++ overflows to 0.

This will not only call deposit on the wrong index, but also leads to an infinite loop. Note that this problem exists regardless of the MAX token cap of 255. Because, technically one can deposit the same token more than 255 times (although it may not be logical to do so).

Recommended Mitigation Steps

• Either add a require(_tokens.length < 255) check
• Or, change uint8 i to uint i.

Handle

hrkrshnn

Vulnerability details

The function removeToken can get prohibitively expensive

The function removeToken loops over a dynamic array, and compares values. This can quickly get expensive as more and more tokens get added. The for loop has two sload operations, assuming cold costs for both these sload, since the maximum number of allowed tokens is 256, the storage costs alone can cost around 1 million gas (2 * 2100 * 256 = 1075200) This is an unsustainable amount of gas for a protocol operation and therefore adding this as a low severity bug instead of as a gas optimization.

One way to prevent this issue is to compute the index off chain and pass it as an argument on chain.

For example:

function removeToken(address _vault, address _token, uint index) public {
    uint256 k = tokens[_vault].length;
    uint256 index;
    bool found;
    require(tokens[_vault][index] == _token);
    // do the removal
}

Alternatively, changing the dynamic array to a mapping can also be considered.