✅RACE #13 - ERC20 with Callback

RACE #13 Of The Secureum Bootcamp Epoch∞ • Ventral DigitalVentral Digital

RACE #13

RACE #13 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: UNLICENSED
pragma solidity ^0.8.17;

/// CallbackERC20 is based on Solmate's ERC20.
/// It adds the extra feature that address can register callbacks,
/// which are called when that address is a recipient or sender
/// of a transfer.
/// Addresses can also revoke callbacks.
contract CallbackERC20 {
   event Transfer(address indexed from, address indexed to, uint256 amount);

   event Approval(address indexed owner, address indexed spender, uint256 amount);

   /*//////////////////////////////////////////////////////////////
                           METADATA STORAGE
   //////////////////////////////////////////////////////////////*/

   // Optimized version vs string
   function name() external pure returns (string memory) {
       // Returns "Callback"
       assembly {
           mstore(0, 0x20)
           mstore(0x28, 0x0843616c6c6261636b)
           return(0, 0x60)
       }
   }

   // Optimized version vs string
   function symbol() external pure returns (string memory) {
       // Returns "CERC"
       assembly {
           mstore(0, 0x20)
           mstore(0x24, 0x0443455243)
           return(0, 0x60)
       }
   }

   uint8 public constant decimals = 18;

   /*//////////////////////////////////////////////////////////////
                             ERC20 STORAGE
   //////////////////////////////////////////////////////////////*/

   uint256 public totalSupply;

   mapping(address => uint256) public balanceOf;

   mapping(address => mapping(address => uint256)) public allowance;

   /*//////////////////////////////////////////////////////////////
                              CALLBACK
   //////////////////////////////////////////////////////////////*/

   mapping(address => function (address, address, uint) external) public callbacks;

   /*//////////////////////////////////////////////////////////////
                              CONSTRUCTOR
   //////////////////////////////////////////////////////////////*/

   constructor() {
       // Owner starts with a little fortune they can distribute.
       _mint(msg.sender, 1_000_000);
   }

   /*//////////////////////////////////////////////////////////////
                             CALLBACK LOGIC
   //////////////////////////////////////////////////////////////*/

  function registerCallback(function (address, address, uint) external callback) external {
      callbacks[msg.sender] = callback;
  }

  function unregisterCallback() external {
      delete callbacks[msg.sender];
  }

   /*//////////////////////////////////////////////////////////////
                              ERC20 LOGIC
   //////////////////////////////////////////////////////////////*/

   function approve(address spender, uint256 amount) public virtual returns (bool) {
       allowance[msg.sender][spender] = amount;

       emit Approval(msg.sender, spender, amount);

       return true;
   }

   function transfer(address to, uint256 amount) public virtual returns (bool) {
       balanceOf[msg.sender] -= amount;

       // Cannot overflow because the sum of all user
       // balances can't exceed the max uint256 value.
       unchecked {
           balanceOf[to] += amount;
       }

       notify(msg.sender, msg.sender, to, amount);
       notify(to, msg.sender, to, amount);

       emit Transfer(msg.sender, to, amount);

       return true;
   }

   function transferFrom(
       address from,
       address to,
       uint256 amount
   ) public virtual returns (bool) {
       uint256 allowed = allowance[from][msg.sender]; // Saves gas for limited approvals.

       if (allowed != type(uint256).max) allowance[from][msg.sender] = allowed - amount;

       balanceOf[from] -= amount;

       // Cannot overflow because the sum of all user
       // balances can't exceed the max uint256 value.
       unchecked {
           balanceOf[to] += amount;
       }

       notify(from, from, to, amount);
       notify(to, from, to, amount);

       emit Transfer(from, to, amount);

       return true;
   }

   function notify(address who, address from, address to, uint amt) internal {
       if (callbacks[who].address != address(0)) {
           callbacks[who](from, to, amt);
       }
   }

   /*//////////////////////////////////////////////////////////////
                       INTERNAL MINT/BURN LOGIC
   //////////////////////////////////////////////////////////////*/

   function _mint(address to, uint256 amount) internal virtual {
       totalSupply += amount;

       // Cannot overflow because the sum of all user
       // balances can't exceed the max uint256 value.
       unchecked {
           balanceOf[to] += amount;
       }

       emit Transfer(address(0), to, amount);
   }

   function _burn(address from, uint256 amount) internal virtual {
       balanceOf[from] -= amount;

       // Cannot underflow because a user's balance
       // will never be larger than the total supply.
       unchecked {
           totalSupply -= amount;
       }

       emit Transfer(from, address(0), amount);
   }
}

Question 1 ✅

In transferFrom(), unchecked is not used in the allowance subtraction:

• A. To save gas

• B. To avoid unauthorized transfers -> Might underflow to unlimited allowance

• C. To avoid reentrancy

• D. None of the above

Question 2 - This one tricky

In transfer() and transferFrom(), the use of unchecked would not be desired:

• A. When the token uses large number of decimals

• B. When the token uses small number of decimals

• C. At all times

• D. At no times

Comment:

Originally, the correct answer was intended to be A with the following explanation from Leo: "Unchecked would be always desired if this operation can never overflow, and never desired if it can easily overflow. Neither case is true. The number of decimals can influence that. The more decimals the token uses, the bigger one's balance is, in terms of tokens. Depending on how large decimals is, it can lead to the case where overflow is possible in a legit use case. In this case, unchecked could lead to bugs."

This assumed though, that the operation can overflow, which it actually can't in the context of this contract since, as mentioned, by the code-comment "Cannot overflow because the sum of all user balances can't exceed the max uint256 value." thanks to the _mint() function's totalSupply increase not being in an unchecked block.

Although the use of large decimals doesn't negatively impact the token's own logic, it should still be mentioned that it might cause issues for other contracts that would like to integrate the tokens. An example for this would be the multiplication of two user's balances where, as per best practice, multiplication would happen before division (to avoid loss of precision) and might cause an overflow.

Question 3 - This one tricky

In name() and symbol(), the returned values are incorrect because:

• A. The string encoding is too short

• B. Inline assembly return does not leave the function

• C. MSTORE does not fill all bytes until 0x5f and function may return junk at the end -> Free memory pointer chunk ends at 0x5f

• D. The code always reverts

Comment:

The first MSTORE writes a static 0x20 (32 in decimal) to the first memory slot. This is done since the RETURNed value is expected to be ABI-encoded and the encoding of dynamic types such as string is an offset that points to where the actual string in the return data starts. Since there's no other data being returned, it simply points to the next 32-byte slot starting at address 0x20.

The second MSTORE actually writes two things at the same time: The string length and the actual string contents. Strings always consist of these two parts and it's important to remember that the length is right-aligned (like all numbers in memory) in the first slot, while the string's content is left-aligned (like all strings and dynamic byte-arrays in memory) in the following slots.

Also, remember that MSTORE always writes 32 bytes of data starting at the specified address even when the data you specified is shorter than 32 bytes. In such cases, the data will be left-padded to automatically right-align it like a number. Both functions make use of this fact by not starting at 0x20 but instead increasing the address by the length of the string. That way the first byte of the string in the code will end up at the last byte of the slot while the following bytes are moved to the beginning of the next slot.

While this seems like an elegant approach, the problem now is that only the first few bytes (the string contents) of the third slot have been written to. All of the following bytes are simply assumed to be zero-values although they might contain junk which could cause issues for the receiver of the returned data. In this specific case, the code accesses the memory area where Solidity stores the "free memory pointer". This value is returned as part of the string, which is basically junk.

Finally, the RETURN operation in inline assembly not only leaves the function, but stops the execution of the transaction. It's is told to return 0x60 (3 * 32) bytes starting at offset 0x0, effectively returning all of the memory slots that had been written to:

0000000000000000000000000000000000000000000000000000000000000020 // From first MSTORE, ABI encoding representing address where the string starts
0000000000000000000000000000000000000000000000000000000000000008 // First byte from second MSTORE, the string's length
43616c6c6261636b000000000000000000000000000000000000000000000080 // Rest of bytes from second MSTORE that moved into next slot, the string's content
//                                                            ^^ Junk from the free memory pointer

Question 4 ✅

To correct name(), one could make the following change(s):

• A

assembly {
    mstore(0x20, 0x20)
    mstore(0x48, 0x0843616c6c6261636b)
    return(0x20, 0x60)

• B

function name() external pure returns (string memory n) { n = "Callback"; }

• C

return "Callback";

• D. None of the above

Comment:

Answer A moves the memory used for the return value by one 32-byte slot "to the right". This way the string won't end up sharing its space with where Solidity stores the "free memory pointer" and no junk will be returned anymore, at least in this specific case. It still clashes with other reserved memory areas of Solidity, but since they are zero in this case it doesn't matter. The fact that reserved memory areas are used also is no problem as long as control is never returned back to Solidity, as is the case here thanks to the use of RETURN.

Answers B and C are basically equivalent and represent the usual ways one would do it in Solidity.

Question 5 ✅

The concern(s) with the check in notify() is/are:

• A. Selector 0x00000000 is the fallback function

• B. Selector 0x00000000 is the receive function

• C. Selector 0x00000000 is possible in which case a valid callback would not be called

• D. Selector can never be 0x00000000, so the check is useless

Comment:

A and B are simply not true, as fallback and receive don't have selectors. Rather, they are called when no function with the requested selector was implemented in the called contract.

Answer C is regarding the possibility that, although unlikely, a normal external function could end up having 0x00000000 as function selector. In such a case, Solidity will revert (thinking that the variable was not properly initialized) when attempting to call said callback. Therefore a valid callback would not be called.

Question 6 ✅

The concern(s) with the call in notify() is/are:

• A. The call always reverts

• B. The passed function pointer is internal and therefore not accessible via an external call

• C. One should always use try/catch in external calls

• D. The called contract may not have the called function selector thus falling through to fallback or reverting the transfer

Question 7 ✅

Potential change(s) to notify() to mitigate further security concern(s) is/are:

• A. Enforce the callback call to use delegatecall

• B. Enforce the callback call to use staticcall

• C. Send Ether to the called contract

• D. Make the call in inline assembly

Question 8 ✅

How can the contract be exploited for loss-of-funds via notify callback reentrancy?

• A. During the callback, while being the sender of a transfer, repeat the transfer

• B. During the callback, while being the recipient of a transfer, call transfer again in the token contract sending the tokens back to the original sender

• C. During the callback, while being the recipient of a transfer, burn the received tokens

• D. This cannot happen in this contract