Sushi Miso contest - hrkrshnn's results

Handle

hrkrshnn

Vulnerability details

An adversarial attacker can initialize ListFactory

The ListFactory contract has a default constructor. However, the proper initialization of state variables is done in initListFactor. This makes it vulnerable to front-running as well as a targeted attack by an adversary.

Here is an example flow of events:

1. Alice deploys ListFactory
2. Alice calls ListFactory.initListFactory(_accessControls, _pointListTemplate, _minimumFee).
3. Bob can watch the above tx in the mempool and frontrun it changing the values of _accessControls, _pointListTemplate and _minimumFee to whatever they please.
4. Alice's initListFactory reverts.

If Alice is not careful enough to check if their call succeeded, then they might use an incorrectly initialized ListFactory contract.

A second flow of events:

1. Alice deploys ListFactory
2. Bob watches the mempool for a create tx with bytecode that matches the bytecode of ListFactory (Bob has to plan this in advance).
3. Bob sends an ListFactory.initListFactory(_accessControls, _pointListTemplate, _minimumFee) to immediately follow Alice's ListFactory deployment. Tools such as flashbots allow doing this precisely.

Alice's deployed contract is now useless, since the parameters were initialized by Bob. Bob can effectively deny Alice from deploying a useful version of ListFactory.

Note that there are several other places, where this pattern is used, for example in MISOAccessFactory (generally, function names that match init* would all fall under this category.)

Recommended Mitigation Steps

• Consider doing the initialization in the constructor code itself. Implementing this is easy and quintessential. Doing it in the constructor also opens up for some gas optimization as well: for example, the state variable accessControls and pointListTemplate can be made immutable.
• Or, Deploy ListFactory from another smart contract using a create call, followed by initListFactory(...) in the same transaction.

If the latter approach is used, consider explicitly specifying this in the contract documentation.

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/Access/PointList.sol:68:        for (uint i = 0; i < _accounts.length; i++) {
./contracts/Helper/MISOHelper.sol:91:        for (uint256 i = 0; i < addresses.length; i++) {
./contracts/Helper/MISOHelper.sol:567:        for (uint256 i = 0; i < markets.length; i++) {
./contracts/OpenZeppelin/access/TimelockController.sol:67:        for (uint256 i = 0; i < proposers.length; ++i) {
./contracts/OpenZeppelin/access/TimelockController.sol:72:        for (uint256 i = 0; i < executors.length; ++i) {
./contracts/OpenZeppelin/access/TimelockController.sol:190:        for (uint256 i = 0; i < targets.length; ++i) {
./contracts/OpenZeppelin/access/TimelockController.sol:250:        for (uint256 i = 0; i < targets.length; ++i) {
./contracts/UniswapV2/UniswapV2Library.sol:66:        for (uint i; i < path.length - 1; i++) {
./contracts/UniswapV2/UniswapV2Library.sol:77:        for (uint i = path.length - 1; i > 0; i--) {
./contracts/Utils/BoringBatchable.sol:38:        for (uint256 i = 0; i < calls.length; i++) {

Handle

hrkrshnn

Vulnerability details

Consider having short revert strings

Consider the following require statement:

// condition is boolean
// str is a string
require(condition, str)

The string str is split into 32-byte sized chunks and then stored in memory using mstore, then the memory offsets are provided to revert(offset, length). For chunks shorter than 32 bytes, and for low --optimize-runs value (usually even the default value of 200), instead of push32 val, where val is the 32 byte hexadecimal representation of the string with 0 padding on the least significant bits, the solidity compiler replaces it by shl(value, short-value)). Where short-value does not have any 0 padding. This saves the total bytes in the deploy code and therefore saves deploy time cost, at the expense of extra 6 gas during runtime. This means that shorter revert strings saves deploy time costs of the contract. Note that this kind of saving is not relevant for high values of --optimize-runs as push32 value will not be replaced by a shl(..., ...) equivalent by the Solidity compiler.

Going back, each 32 byte chunk of the string requires an extra mstore. That is, additional cost for mstore, memory expansion costs, as well as stack operations. Note that, this runtime cost is only relevant when the revert condition is met.

Overall, shorter revert strings can save deploy time as well as runtime costs.

Note that if your contracts already allow using at least Solidity 0.8.4, then consider using Custom errors. This is more gas efficient, while allowing the developer to describe the errors in detail using NatSpec. A disadvantage to this approach is that, some tooling may not have proper support for this, as well as block explorers like Etherscan.

Examples

Here is a non-exhaustive list of lengthy revert strings that could be shortened:

1. https://github.com/sushiswap/miso/blob/master/contracts/Auctions/BatchAuction.sol#L136
2. https://github.com/sushiswap/miso/blob/master/contracts/Auctions/BatchAuction.sol#L137
3. https://github.com/sushiswap/miso/blob/master/contracts/Auctions/BatchAuction.sol#L183

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.

Note that in older Solidity versions, changing some function arguments from memory to calldata may cause "unimplemented feature error". This can be avoided by using a newer (0.8.*) Solidity compiler.

(non-exhaustive) List of Examples

1. https://github.com/sushiswap/miso/blob/2cdb1486a55ded55c81898b7be8811cb68cfda9e/contracts/Access/PointList.sol#L64
2. https://github.com/sushiswap/miso/blob/2cdb1486a55ded55c81898b7be8811cb68cfda9e/contracts/Access/ListFactory.sol#L151
3. https://github.com/sushiswap/miso/blob/2cdb1486a55ded55c81898b7be8811cb68cfda9e/contracts/Access/ListFactory.sol#L152
4. https://github.com/sushiswap/miso/blob/2cdb1486a55ded55c81898b7be8811cb68cfda9e/contracts/Access/PointList.sol#L64

Handle

hrkrshnn

Vulnerability details

Caching totalPoints during setPoints method

Instead of constantly writing to the same slot in a for loop, write it once at the end. This would save 100 gas for each iteration of the for loop. (Since EIP-2929, the cost of writing to a dirty storage slot is 100 gas)

modified   contracts/Access/PointList.sol
@@ -65,6 +65,7 @@ contract PointList is IPointList, MISOAccessControls {
    function setPoints(address[] memory _accounts, uint256[] memory _amounts) external override {
         require(hasAdminRole(msg.sender) || hasOperatorRole(msg.sender), "PointList.setPoints: Sender must be operator");
         require(_accounts.length != 0, "PointList.setPoints: empty array");
         require(_accounts.length == _amounts.length, "PointList.setPoints: incorrect array length");
+        uint totalPointsCache = totalPoints;
         for (uint i = 0; i < _accounts.length; i++) {
             address account = _accounts[i];
             uint256 amount = _amounts[i];
@@ -72,9 +73,10 @@ contract PointList is IPointList, MISOAccessControls {

             if (amount != previousPoints) {
                 points[account] = amount;
-                totalPoints = totalPoints.sub(previousPoints).add(amount);
+                totalPointsCache = totalPointsCache.sub(previousPoints).add(amount);
                 emit PointsUpdated(account, previousPoints, amount);
             }
         }
+        totalPoints = totalPointsCache;
     }
 }

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.