Sherlock contest - robee's results

Handle

robee

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:


    SherlockClaimManager.priceDisputed (request)
    SherlockClaimManager.priceSettled (ancillaryData)
    SherlockClaimManager.priceProposed (ancillaryData)
    SherlockClaimManager.priceProposed (request)
    SherlockClaimManager.priceDisputed (ancillaryData)
    Sherlock.constructor (_name)
    Sherlock.constructor (_symbol)

Handle

robee

Vulnerability details

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory We recommend to change from: for (uint256 i=0; i<array.length; i++) { ... } to: uint len = array.length for (uint256 i=0; i<len; i++) { ... } These functions use not using prefix increments (++x) or not using the unchecked keyword:

    Manager.sol, _extraTokens, 46
    SherlockClaimManager.sol, claimCallbacks, 234
    SherlockClaimManager.sol, claimCallbacks, 505
    SherlockProtocolManager.sol, _protocol, 729
    SherBuy.sol, _tokens, 186
    Sherlock.sol, _initialstakingPeriods, 106

Handle

robee

Vulnerability details

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: Manager.sol, i, 46
    change to prefix increment and unchecked: SherlockClaimManager.sol, i, 234
    change to prefix increment and unchecked: SherlockClaimManager.sol, i, 505
    change to prefix increment and unchecked: SherlockProtocolManager.sol, i, 729
    change to prefix increment and unchecked: SherBuy.sol, i, 186
    change to prefix increment and unchecked: Sherlock.sol, i, 106