Kuiper contest - hrkrshnn's results

Handle

hrkrshnn

Vulnerability details

Proposals can never get created due to reaching block.gaslimit

The function proposeBasketLicense allows initializing proposals of with arbitrary amount of tokens. However, createBasket stage involves the actual transfers. Since each unique token in the list undergoes a safeApprove, which would cost at least 22,100 gas (for zero to non-zero sstore update). Taking this alone would mean that having a token list of size 1300 would exceed the current block gas limit. This number would in practice be even lower when including other calls.

Recommended Mitigation Steps

1. Try to measure the cost of proposeBasketLicense for n tokens and try to estimate n that exceeds the current block gas limit.
2. Hardcode this value (or lower) in proposeBasketLicense with a require(tokens.length < n).

This would more or less guarantee that each proposed basket can be created.

Handle

hrkrshnn

Vulnerability details

Sanity checks when the contract parameters are updated

The protocol defines several parameters, for example, minLicenseFee which can be updated by using a corresponding setter function that the owner specifies.

However, there are no sanity checks for these parameters in the setter functions. Without sanity checks, the owner can set the parameters in a way to sabotage the protocol.

For example, setting auctionDecrement to 0 would mean that settleAuction function will always revert, making some operations in the protocol to halt. Therefore, adding a check that the value is non-zero in the setter avoids potential problems of this sort.

Recommended Mitigation Steps

For each parameter, add a require check for both upper and lower-bounds.

Handle

hrkrshnn

Vulnerability details

Replace tokenList.length by existing variable length

modified   contracts/contracts/Basket.sol
@@ -61,7 +61,7 @@ contract Basket is IBasket, ERC20Upgradeable {
             require(_tokens[i] != address(0));
             require(_weights[i] > 0);

-            for (uint256 x = 0; x < tokenList.length; x++) {
+            for (uint256 x = 0; x < length; x++) {
                 require(_tokens[i] != tokenList[x]);
             }

Context: https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L64

The value tokenList.length is read from memory and therefore requires a mload(...) (6 gas for push memory_offset + mload). On the other hand, this value is already available in the stack as length and could just be dup-ed (3 gas). Saves 3 gas for each loop iteration of the interior loop.

Handle

hrkrshnn

Vulnerability details

State variables that can be set to immutable

Solidity 0.6.5 introduced immutable as a major feature. It allows setting contract-level variables at construction time which gets stored in code rather than storage.

Consider the following generic example:

contract C {
    /// The owner is set during contruction time, and never changed afterwards.
    address public owner = msg.sender;
}

In the above example, each call to the function owner() reads from storage, using a sload. After EIP-2929, this costs 2100 gas cold or 100 gas warm. However, the following snippet is more gas efficient:

contract C {
    /// The owner is set during contruction time, and never changed afterwards.
    address public immutable owner = msg.sender;
}

In the above example, each storage read of the owner state variable is replaced by the instruction push32 value, where value is set during contract construction time. Unlike the last example, this costs only 3 gas.

Examples

A non-exhaustive list of examples:

1. https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Factory.sol#L26
2. https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Factory.sol#L27
3. https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L22
4. https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L23

Handle

hrkrshnn

Vulnerability details

The increment in for loop post condition can be made unchecked

(This is only relevant if you are using the default solidity checked arithmetic.)

Consider the following generic for loop:

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

In this example, the for loop post condition, i.e., i++ involves checked arithmetic, which is not required. This is because the value of i is always strictly less than length <= 2**256 - 1. Therefore, the theoretical maximum value of i to enter the for-loop body is `2**256

• 2. This means that the i++` in the for loop can never overflow. Regardless, the overflow checks are performed by the compiler.

Unfortunately, the Solidity optimizer is not smart enough to detect this and remove the checks. One can manually do this by:

for (uint i = 0; i < length; i = unchecked_inc(i)) {
    // do something that doesn't change the value of i
}

function unchecked_inc(uint i) returns (uint) {
    unchecked {
        return i + 1;
    }
}

Note that it's important that the call to unchecked_inc is inlined. This is only possible for solidity versions starting from 0.8.2.

Gas savings: roughly speaking this can save 30-40 gas per loop iteration. For lengthy loops, this can be significant!

Examples

./contracts/contracts/Auction.sol:81:        for (uint256 i = 0; i < inputTokens.length; i++) {
./contracts/contracts/Auction.sol:85:        for (uint256 i = 0; i < outputTokens.length; i++) {
./contracts/contracts/Auction.sol:96:        for (uint256 i = 0; i < pendingWeights.length; i++) {
./contracts/contracts/Auction.sol:142:        for (uint256 i = 0; i < bountyIds.length; i++) {
./contracts/contracts/Factory.sol:103:        for (uint256 i = 0; i < bProposal.weights.length; i++) {
./contracts/contracts/Basket.sol:60:        for (uint i = 0; i < length; i++) {
./contracts/contracts/Basket.sol:225:        for (uint256 i = 0; i < weights.length; i++) {
./contracts/contracts/Basket.sol:231:        for (uint256 i = 0; i < weights.length; i++) {
./contracts/contracts/Basket.sol:238:        for (uint256 i = 0; i < weights.length; i++) {

Handle

hrkrshnn

Vulnerability details

Use calldata instead of memory for function parameters

In some cases, having function arguments in calldata instead of memory is more optimal.

Consider the following generic example:

contract C {
    function add(uint[] memory arr) external returns (uint sum) {
        uint length = arr.length;
        for (uint i = 0; i < arr.length; i++) {
            sum += arr[i];
        }
    }
}

In the above example, the dynamic array arr has the storage location memory. When the function gets called externally, the array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). And during the for loop, arr[i] accesses the value in memory using a mload. However, for the above example this is inefficient. Consider the following snippet instead:

contract C {
    function add(uint[] calldata arr) external returns (uint sum) {
        uint length = arr.length;
        for (uint i = 0; i < arr.length; i++) {
            sum += arr[i];
        }
    }
}

In the above snippet, instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

Gas savings: In the former example, the ABI decoding begins with copying value from calldata to memory in a for loop. Each iteration would cost at least 60 gas. In the latter example, this can be completely avoided. This will also reduce the number of instructions and therefore reduces the deploy time cost of the contract.

In short, use calldata instead of memory if the function argument is only read.

Examples

1. https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Factory.sol#L65
2. https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L69
3. https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L140
4. https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L36
5. https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L53
6. https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Basket.sol#L170