✅RACE #10 - Test Cases

Secureum Bootcamp Epoch∞ - October RACE #10 • Ventral DigitalVentral Digital

RACE #10

RACE #10 result

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.

// 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 ✅

Which statements are true in Test1()?

• A. The function does not use all supplied extra data -> Not sure what this means

• B. The function can revert due to an underflow -> msg.data.length-64

• C. The function can revert due to an overflow -> n + ...

• 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 ✅

Which statements are true in Test2()?

• A. Result of encodePacked is deterministic -> Of course abi.encodePacked(uint(4),uint(5)) is deterministic

• B. abi.decode always succeeds -> Only succeeds when fcall returns legit data, otherwise the staticcall will fail

• C. It calls function Test1() without any problem -> abi.encodeCall(TestContract.Test1,(n)) is good

• 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. One way to cause this to happen would be supplying a number n that causes an overflow. The best practice is to check the success boolean before attempting to decode the returned data.

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 ✅

Which statements are true in NextBookNonce()?

• A. wrap and unwrap cost additional gas -> No effect from EVM's perspective

• B. It is safe to use unchecked -> x.nonce + 3 is a small increment, exploiting this is expensive

• C. There is something suspicious about the increment value -> Nonce usually increments by 1

• D. It could have used x.nonce++

Comment:

The calls to wrap and unwrap are basically telling Solidity whether it should treat a certain variable as being of a custom type (Nonce) or of its native type (uint256). 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.

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 ✅

Which statements are true in Test3()?

• A. bookIndex.nonce is incremented in the loop -> NextBookNonce() is called in the for loop

• B. bookIndex.nonce cannot be incremented because NextBookNonce is pure -> bookIndex is stored in memory, so pure function can modify it

• 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.

• 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 ✅

Which statements are true In Test4()?

• A. The function always reverts with ZeroAddress()

• B. The function always reverts with ZeroAmount()

• C. The function never reverts with ZeroAddress()

• 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:

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 ✅

Which statements are true in Test5()?

• A. modifier checkInvariants will pause the contract if too much is minted

• B. modifier checkInvariants will never pause the contract

• C. modifier checkInvariants will always pause the contract

• 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.

A single call to require would fix this issue and also be more efficient.

My comment:

The modifier checkInvariants() has a logic issue:

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 ✅

Which statements are true about the owner?

• A. The owner is initialized -> owner is inherited from Ownable

• B. The owner is not initialized

• C. The owner cannot be changed

• 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 ✅

Which statements are true in Test5() and related functions?

• A. pause is unsafe -> onlyOwner modifier is missing

• B. unpause is unsafe

• C. The emit is done right

• 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.