# RACE #10 - Test Cases

{% embed url="https://ventral.digital/posts/2022/10/3/secureum-bootcamp-epoch-october-race-10" %}
RACE #10
{% endembed %}

<figure><img src="https://3988450783-files.gitbook.io/~/files/v0/b/gitbook-x-prod.appspot.com/o/spaces%2F-MWVjG_njKgBtvmnKaJh%2Fuploads%2FMcV5lJbbr41Q9EdYzK7Q%2Fimage.png?alt=media&#x26;token=dc788e06-185a-41ca-8d8c-d9f468c371a5" alt=""><figcaption><p>RACE #10 result</p></figcaption></figure>

{% hint style="info" %}
_Note: All 8 questions in this RACE are based on the below contract. This is the same contract you will see for all the 8 questions in this RACE. The question is below the shown contract._
{% endhint %}

```solidity
// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;
import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";

contract TestContract is Ownable {

    function Test1(uint n) external pure returns(uint) {
        return n + abi.decode(msg.data[msg.data.length-64:], (uint));
    }

    function Test2(uint n) public view returns(uint) {
        bytes memory fcall = abi.encodeCall(TestContract.Test1,(n));
        bytes memory xtr = abi.encodePacked(uint(4),uint(5));
        bytes memory all = bytes.concat(fcall,xtr);
        (bool success, bytes memory data) = address(this).staticcall(all);
        return abi.decode(data,(uint));
    }

    type Nonce is uint256;
    struct Book { Nonce nonce;}

    function NextBookNonce(Book memory x) public pure {
       x.nonce = Nonce.wrap(Nonce.unwrap(x.nonce) + 3);
    }

    function Test3(uint n) public pure returns (uint) {
      Book memory bookIndex;
      bookIndex.nonce = Nonce.wrap(7);
      for (uint i=0;i<n;i++) {
         NextBookNonce(bookIndex);
      }
      return Nonce.unwrap(bookIndex.nonce);
    }

    error ZeroAddress();
    error ZeroAmount();
    uint constant ZeroAddressFlag = 1;
    uint constant ZeroAmountFlag = 2;

    function process(address[] memory a, uint[] memory amount) public pure returns (uint){
        uint error;
        uint total;
        for (uint i=0;i<a.length;i++) {
            if (a[i] == address(0)) error |= ZeroAddressFlag;
            if (amount[i] == 0) error |= ZeroAmountFlag;
            total += amount[i];
        }
        if (error == ZeroAddressFlag) revert ZeroAddress();
        if (error == ZeroAmountFlag)  revert ZeroAmount();
        return total;
    }

    function Test4(uint n) public pure returns (uint) {
        address[] memory a = new address[](n+1);
        for (uint i=0;i<=n;i++) {
            a[i] = address(uint160(i));
        }
        uint[] memory amount = new uint[](n+1);
        for (uint i=0;i<=n;i++) {
            amount[i] = i;
        }
        return process(a,amount);
    }

    uint public totalMinted;
    uint constant maxMinted = 100;
    event minted(uint totalMinted,uint currentMint);

    modifier checkInvariants() {
        require(!paused, "Paused");
        _;
        invariantCheck();
        require(!paused, "Paused");
    }

    function invariantCheck() public {
        if (totalMinted > maxMinted) // this may never happen
            pause();
    }

    bool public paused;
    function pause() public {
        paused = true;
    }
    function unpause() public onlyOwner {
        paused = false;
    }

    function Test5( uint n) public checkInvariants(){
        totalMinted += n;
        emit minted(n,totalMinted);
    }
}
```

## Question 1 :white\_check\_mark:

Which statements are true in _Test1()_?

* [x] &#x20;A. The function does not use all supplied extra data -> <mark style="color:red;">**Not sure what this means**</mark>
* [x] &#x20;B. The function can revert due to an underflow -> `msg.data.length-64`
* [x] &#x20;C. The function can revert due to an overflow -> `n + ...`
* [ ] &#x20;D. The function accesses memory which it should not

**Comment:**

Answer A seems a bit confusing when looking at _Test1()_ alone, but seeing the _xtr_ variable of _Test2()_ brings some clarity: The _Test1()_ function signature expects one uint to be passed, but then within the function body it loads 64 bytes directly from calldata. _Test2()_ then shows how the function is intended to be called by concatenating extra data to the ABI encoded calldata. It adds two more uint types which together are 64 bytes of extra data. But then in the _abi.decode_ only the first uint from extra data is actually decoded and used.

Both B and C are true since a Solidity version (^0.8.0) is used, that automatically checks for integer over/underflows and reverts when these happen. In this specific case, an overflow could happen when parameter _n_ or the number supplied from extra data are large enough to wrap. The underflow can happen when the overall supplied calldata is smaller than 64 bytes, making the subtraction within the slicing parameters fail.

You could argue that accessing _msg.data_ directly should be avoided when possible. But this doesn't access memory but read-only calldata. Therefore no memory is accessed that should not be.

## Question 2 :white\_check\_mark:

Which statements are true in _Test2()_?

* [x] &#x20;A. Result of _encodePacked_ is deterministic -> Of course `abi.encodePacked(uint(4),uint(5))` is deterministic
* [ ] &#x20;B. _abi.decode_ always succeeds -> Only succeeds when `fcall` returns legit data, otherwise the staticcall will fail
* [x] &#x20;C. It calls function _Test1()_ without any problem -> `abi.encodeCall(TestContract.Test1,(n))` is good
* [ ] &#x20;D. It should use _uint256_ instead of _uint ->_ `uint` is just an alias for `uint256`

**Comment:**

Deterministic means that you should always get the same predictable output for a given input. As such, _encodePacked_ always encodes passed data the same way.

Test2's _abi.decode_ will only succeed if no error happens in _Test1()_. If _Test1()_ reverts the returned data would not contain a decodable uint but error data. <mark style="color:red;">**One way to cause this to happen would be supplying a number**</mark><mark style="color:red;">**&#x20;**</mark>_<mark style="color:red;">**n**</mark>_<mark style="color:red;">**&#x20;**</mark><mark style="color:red;">**that causes an overflow. The best practice is to check the**</mark><mark style="color:red;">**&#x20;**</mark>_<mark style="color:red;">**success**</mark>_<mark style="color:red;">**&#x20;**</mark><mark style="color:red;">**boolean before attempting to decode the returned data.**</mark>

Answer D leaves some room for interpretation. _uint_ is an alias of _uint256_ and there should not be an issue using it here. But it's a common best practice to avoid the shorter alias and instead use the longer-named version of the type. While this is generally considered to improve readability, I'd argue that consistency (always using the same type) is more important.

## Question 3 :white\_check\_mark:

Which statements are true in _NextBookNonce()_?

* [ ] &#x20;A. _wrap_ and _unwrap_ cost additional gas -> No effect from EVM's perspective
* [x] &#x20;B. It is safe to use _unchecked ->_ `x.nonce + 3` is a small increment, exploiting this is expensive
* [x] &#x20;C. There is something suspicious about the increment value -> Nonce usually increments by 1
* [ ] &#x20;D. It could have used _x.nonce++_

**Comment:**

<mark style="color:red;">**The calls to**</mark><mark style="color:red;">**&#x20;**</mark>_<mark style="color:red;">**wrap**</mark>_<mark style="color:red;">**&#x20;**</mark><mark style="color:red;">**and**</mark><mark style="color:red;">**&#x20;**</mark>_<mark style="color:red;">**unwrap**</mark>_<mark style="color:red;">**&#x20;**</mark><mark style="color:red;">**are basically telling Solidity whether it should treat a certain variable as being of a custom type (**</mark>_<mark style="color:red;">**Nonce**</mark>_<mark style="color:red;">**) or of its native type (**</mark>_<mark style="color:red;">**uint256**</mark>_<mark style="color:red;">**). This switch is basically just syntactic sugar for handling types within Solidity, the EVM will know nothing of these type switches and no additional gas will be used by doing so.**</mark>

Using an _unchecked_ block in this function would omit Solidity's over/underflow handling. Especially in the context of a Nonce (Number used only once), you don't want integer values to wrap and overflow to values that were once used before. But usually, a nonce is only increased by such a small value that exploiting this would be very expensive. In this specific case, the function is pure and the nonce is not stored, so whether it's safe to use unchecked block will depend on the function being used correctly.

Answer C sounds rather ominous but it's simply pointing out that Nonces are commonly increased by one and not by such a weird number as 3.

Arithmetic operations cannot be executed on custom types without unwrapping the number first.

## Question 4 :white\_check\_mark:

Which statements are true in _Test3()_?

* [x] &#x20;A. _bookIndex.nonce_ is incremented in the loop -> `NextBookNonce()` is called in the for loop
* [ ] &#x20;B. _bookIndex.nonce_ cannot be incremented because _NextBookNonce_ is pure -> `bookIndex` is stored in memory, so pure function can modify it
* [x] &#x20;C. _i++_ can be made _unchecked ->_ `i++` won't overflow because `NextBookNonce()` increments by 3 on each step and `i++` increments by 1 on each step. If overflow occurs, `NextBookNonce()` will overflow first and that will be protected by Solidity compiler.
* [ ] &#x20;D. _memory_ can be changed to _storage_ without any other changes

**Comment:**

Both A and B should be clear from reading the code.

The increment of _i_ within the loop can indeed be made _unchecked_ since it won't be able to overflow no matter what is supplied as _n_.

The _memory_ location can't simply be changed to _storage_ without various further changes such as assigning it to a specific storage slot before being able to make use of it.

## Question 5 :white\_check\_mark:

Which statements are true In _Test4()_?

* [ ] &#x20;A. The function always reverts with _ZeroAddress()_
* [ ] &#x20;B. The function always reverts with _ZeroAmount()_
* [x] &#x20;C. The function never reverts with _ZeroAddress()_
* [x] &#x20;D. The function never reverts with _ZeroAmount()_

**Comment:**

The first array elements of both _a_ and _amounts_ will always be zero-like. Both _1_ for _ZeroAddress_ and _2_ for _ZeroAmount_ will be OR-combined resulting in 3. Once this value is set as an error, further iterations will not influence it. After the loop has finished, this error value is not checked for and instead, the function returns the _total_ without reverting.

**My comment:**

Some OR boolean algebra:

```solidity
function process(address[] memory a, uint[] memory amount) public pure returns (uint){
    uint error;
    uint total;
    for (uint i=0;i<a.length;i++) {
        // @audit a[0] and amount[0] are zero-like
        // error = 1 | 2 = 0b01 | 0b10 = 0b11 = 3
        // 3 | 1 = 0b11 | 0b01 = 0b11 = 3
        // 3 | 2 = 0b11 | 0b10 = 0b11 = 3
        if (a[i] == address(0)) error |= ZeroAddressFlag;
        if (amount[i] == 0) error |= ZeroAmountFlag;
        total += amount[i];
    }
    // @audit `error` will be set to 3 for all i >= 1
    // That means the following two if conditions will never be satisfied
    if (error == ZeroAddressFlag) revert ZeroAddress();
    if (error == ZeroAmountFlag)  revert ZeroAmount();
    return total;
}
```

## Question 6 :white\_check\_mark:

Which statements are true in _Test5()_?

* [ ] &#x20;A. modifier _checkInvariants_ will pause the contract if too much is minted
* [x] &#x20;B. modifier _checkInvariants_ will never pause the contract
* [ ] &#x20;C. modifier _checkInvariants_ will always pause the contract
* [x] &#x20;D. There are more efficient ways to handle the _require_

**Comment:**

While the _checkInvariants_ modifier does intend to pause the contract if too much is minted, it'll be unable to ever do so since this will be reverted by the second _require_ call.

<mark style="color:red;">**A single call to**</mark><mark style="color:red;">**&#x20;**</mark>_<mark style="color:red;">**require**</mark>_<mark style="color:red;">**&#x20;**</mark><mark style="color:red;">**would fix this issue and also be more efficient.**</mark>

**My comment:**

The modifier `checkInvariants()` has a logic issue:

```solidity
modifier checkInvariants() {
    require(!paused, "Paused");
    _;
    invariantCheck();
    // @audit Even `pause()` is called in `invariantCheck()`
    // The whole tx will be reverted because of the following require statement
    require(!paused, "Paused");
}
```

## Question 7 :white\_check\_mark:

Which statements are true about the _owner_?

* [x] &#x20;A. The _owner_ is initialized -> `owner` is inherited from Ownable
* [ ] &#x20;B. The _owner_ is not initialized
* [ ] &#x20;C. The _owner_ cannot be changed
* [x] &#x20;D. The _owner_ can be changed -> `transferOwnership()` is inherited from Ownable

**Comment:**

Although not visible here, the _owner_ is indeed initialized by the constructor inherited from _Ownable_, which also comes with functions allowing to change the _owner_ at a later point.

## Question 8 :white\_check\_mark:

Which statements are true in _Test5()_ and related functions?

* [x] &#x20;A. _pause_ is unsafe -> `onlyOwner` modifier is missing
* [ ] &#x20;B. _unpause_ is unsafe
* [ ] &#x20;C. The _emit_ is done right
* [x] &#x20;D. The _emit_ is done wrong -> Should be `emit minted(totalMinted, n)`

**Comment:**

The _pause_ function is missing the _onlyOwner_ modifier allowing anyone to arbitrarily pause the contract.

The _minted_ event's parameters appear to be in the wrong order.
