---
title: SujiTech CertiK Final Comments For SujiTech
tags: final-report
---


---

<div align="center" style="font-size:30px;">Final Comments</div>

### <div align="center" style="font-size:30px;">Security Assessment</div>

<div align="center">October 19th, 2020</div>

<div align="center">For: Sujitech, LLC</div>

<div align="center">By: [e-mail]</div>

<div align="center"> [e-mail]</div>

<div style="page-break-after:always;"></div>

## Disclaimer

CertiK reports are not, nor should be considered, an “endorsement” or “disapproval” of any particular project or team. These reports are not, nor should be considered, an indication of the economics or value of any “product” or “asset” created by any team or project that contracts CertiK to perform a security review.

CertiK Reports do not provide any warranty or guarantee regarding the absolute bug-free nature of the technology analyzed, nor do they provide any indication of the technologies proprietors, business, business model or legal compliance.

CertiK Reports should not be used in any way to make decisions around investment or involvement with any particular project. These reports in no way provide investment advice, nor should be leveraged as investment advice of any sort.

CertiK Reports represent an extensive auditing process intending to help our customers increase the quality of their code while reducing the high level of risk presented by cryptographic tokens and blockchain technology.

Blockchain technology and cryptographic assets present a high level of ongoing risk. CertiK’s position is that each company and individual are responsible for their own due diligence and continuous security. CertiK’s goal is to help reduce the attack vectors and the high level of variance associated with utilizing new and consistently changing technologies, and in no way claims any guarantee of security or functionality of the technology we agree to analyze.

### What is a CertiK report?

- A document describing in detail an in depth analysis of a particular piece(s) of source code provided to CertiK by a Client.
- An organized collection of testing results, analysis and inferences made about the structure, implementation and overall best practices of a particular piece of source code.
- Representation that a Client of CertiK has indeed completed a round of auditing with the intention to increase the quality of the company/product’s IT infrastructure and or source code.

------

<div style="page-break-after:always;"></div>

## Overview

#### Project Summary

| **Project Name** | [Red Packet](https://sujitech.com/) |
| ---------------- | ------------------------------------------------------------ |
| **Description** | Contract of red packet, provide creation and claim function. This contract can support ETH, ERC20 and ERC721 token |
| **Platform** | Ethereum; Solidity; ERC20; ERC721 |
| **Codebase** | [GitHub Repository](https://github.com/yisiliu/RedPacket/tree/improvement/gas_optimization) |
| **Commits** | [e2798f0cb5e4607eac269586928f38bf3cd747a5](https://github.com/yisiliu/RedPacket/commit/e2798f0cb5e4607eac269586928f38bf3cd747a5) |

#### Audit Summary

| **Delivery Date** | Oct. 19, 2020 |
| ----------------------- | ------------------------------ |
| **Method of Audit** | Static Analysis, Manual Review |
| **Consultants Engaged** | 2 |
| **Timeline** | Oct. 9, 2020 - Oct. 19 2020 |

#### Vulnerability Summary

| **Total Issues** | 11 |
| ----------------------- | ---- |
| **Total Critical** | 0 |
| **Total Major** | 0 |
| **Total Minor** | 4 |
| **Total Informational** | |

#### Executive Summary

Executive SummaryThis report has been prepared for **SujiTech** to discover issues and vulnerabilities in the source code of their **ERC 20/721 Token** as well as any contract dependencies that were not part of an officially recognized library. A comprehensive examination has been performed, utilizing Dynamic Analysis, Static Analysis, and Manual Review techniques.
The auditing process pays special attention to the following considerations:

- Testing the smart contracts against both common and uncommon attack vectors.

- Assessing the codebase to ensure compliance with current best practices and industry standards.

- Ensuring contract logic meets the specifications and intentions of the client.

- Cross referencing contract structure and implementation against similar smart contracts produced by industry leaders.

- Thorough line-by-line manual review of the entire codebase by industry experts.

#### Vulnerability Summary

| **Total Issues** | 11 |
| ----------------------- | ---- |
| **Total Critical** | 0 |
| **Total Major** | 0 |
| **Total Minor** | 4 |
| **Total Informational** | 7 |

------

<div style="page-break-after:always;"></div>

## Findings

| ID | Title | Type | Severity |
| :----- | :------------------------------------------------------- | :------------ | :------------ |
| UNP-01 | Missing reference files | Volatile code | Informational |
| UNP-02 | Verify the valid range | Comparison | Discussion |
| UNP-03 | Optimization loop | Optimization | Discussion |
| UNP-04 | Missing check for number of red packet | Optimization | Informational |
| UNP-05 | Whether function transfer_token can be called externally | Optimization | Discussion |
| UNP-06 | Cannot check contract defined in token address | Optimization | Discussion |
| UNP-07 | Manage permission | Optimization | Discussion |
| UNP-08 | Performs division before multiplication | Optimization | Informational |
| UNP-09 | Whether red packet with 0 amount make sense | Optimization | Discussion |
| UNP-10 | Return value for claimed is not correct | Optimization | Minor |
| UNP-11 | Comparison to boolean constants | Optimization | Informational |
| UNP-12 | Unlock solidity version & versions are different | Optimization | Informational |
| UNP-13 | Public function that could be declared external | Optimization | Informational |
| UNP-14 | Nonexistent function name | Optimization | Minor |
| UNP-15 | Error traversing the array | Optimization | Minor |
| UNP-16 | Too long digits | Optimization | Informational |
| UNP-17 | Redundant approve | Optimization | Informational |

------

<div style="page-break-after:always;"></div>

### UNP-01: Missing reference files

| Type | Severity | Location |
| :-------------- | :------------ | :--------------- |
| Executable code | Informational | redpacket.sol L2 |

#### Description:

The imported files cann’t be found in the project directory.

import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol";
import "openzeppelin-solidity/contracts/token/ERC721/IERC721.sol";
import "openzeppelin-solidity/contracts/math/SafeMath.sol";

#### Recommendation:

Provide complete local imported files, or use online import.

#### Alleviation:

Sujitech confirmed all these imports are typically implementations of openzeppelin.

------

<div style="page-break-after:always;"></div>

### UNP-02: Verify the valid range

| Type | Severity | Location |
| :----------- | :--------- | :----------------------- |
| Optimization | Discussion | redpacket.sol L103, L106 |

#### Description:

function box (uint16 position, uint16 size, uint256 data) internal pure returns (uint256 boxed) {
    return data << (256 - size - position);
}

function unbox (uint256 base, uint16 position, uint16 size) internal pure returns (uint256 unboxed) {
    return (base << position) >> (256 - size);
}

In order to prevent the input parameter from exceeding the range, it is recommended to verify the valid range of the input parameter.

#### Recommendation:

Valid range of the input parameter.

function box (uint16 position, uint16 size, uint256 data) internal pure returns (uint256 boxed) {
    require(validRange(size, data), "error message");
    return data << (256 - size - position);
}

function unbox (uint256 base, uint16 position, uint16 size) internal pure returns (uint256 unboxed) {
    require(validRange(size, base), "error message");
    return (base << position) >> (256 - size);
}

function validRange(uint16 size, uint256 data) public pure returns(bool){
    if (data > 2 ** uint256(size) - 1) {
        return false;
    } else {
        return true;
    }
}

#### Alleviation:

Sujitech resolved in commit 229db2bffe9e2c38ea416164e3a1232347bca8b4.

------

<div style="page-break-after:always;"></div>

### UNP-03: Optimization loop

| Type | Severity | Location |
| :----------- | :--------- | :----------------- |
| Optimization | Discussion | redpacket.sol L162 |

#### Description:

function getTokenIdWithIndex(uint256[] memory array, uint index) 
internal pure returns(uint256[] memory) {
    if (index >= array.length) return array;
    for (uint i = index; i < array.length - 1; i++){
        array[i] = array[i + 1];
    }
    array[array.length - 1] = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;
    return array;
}

When most of the red packets in the array are taken away, there will be a lot of loops that are redundant.

#### Recommendation:

Just traverse the remaining red packets. It’s more efficient gas-wise.

function getTokenIdWithIndex(uint256[] memory array, uint index, uint remained) 
internal pure returns(uint256[] memory) {
    if (index >= array.length) return array;
    require(index < remained, "error message");
    for (uint i = index; i < remained - 1; i++){
        array[i] = array[i + 1];
    }
    array[remained - 1] = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;
    return array;
}

------

<div style="page-break-after:always;"></div>

### UNP-04: Missing check for number of red packet

| Type | Severity | Location |
| :----------- | :------------ | :---------------- |
| Optimization | Informational | redpacket.sol L60 |

#### Description:

The `HappyRedPacket.create_red_packet` function accept parameter `_number` as the number of red packet. But the number was designed as 8 digits stored in packed1 of RedPacket. `_number` may exceed 2^8-1.

#### Recommendation:

add a required validation for _number after `require(__number > 0, "002")` like below:

……
require(_number > 0, "002");
require(_number < 256,"number of packet exceed the max number!");
……

#### Alleviation:

Sujitech resolved in commit 229db2bffe9e2c38ea416164e3a1232347bca8b4.

------

<div style="page-break-after:always;"></div>

### UNP-05: Whether function transfer_token can be called externally

| Type | Severity | Location |
| :----------- | :--------- | :----------------- |
| Optimization | Discussion | redpacket.sol L122 |

#### Description:

The `HappyRedPacket.transfer_token` function was defined as public and payable. Whether this function can be called externally? Whether payable is necessary?

#### Recommendation:

If transfer_token can not be called externally, replace it with internal would be better. If payable is not necessary, remove it would be better, like below.

function transfer_token(uint token_type, address token_address, address sender_address,
    address recipient_address, uint amount, uint256 [] memory erc721_token_ids) internal{

#### Alleviation:

Sujitech resolved in commit 229db2bffe9e2c38ea416164e3a1232347bca8b4.

------

<div style="page-break-after:always;"></div>

### UNP-06: Cannot check contract defined in token address

| Type | Severity | Location |
| :----------- | :---------- | :---------------------- |
| Optimization | Disscussion | redpacket.sol L125,L126 |

#### Description:

if (token_type == 1) {
    require(IERC20(token_address).balanceOf(sender_address) >= amount, "010");
    IERC20(token_address).approve(sender_address, amount);
    IERC20(token_address).transferFrom(sender_address, recipient_address, amount);
}

Whether token have implemented IERC20 & IERC721? Could we take a look at your token contract?

#### Recommendation:

If yes, validation for sender’s balance(L125) and approve function(L126) are redundant, remove them like below:

if (token_type == 1) {
    IERC20(token_address).transferFrom(sender_address, recipient_address, amount);
}

------

<div style="page-break-after:always;"></div>

### UNP-07: Manage permission

| Type | Severity | Location |
| :----------- | :--------- | :---------------- |
| Optimization | Discussion | redpacket.sol L72 |

#### Description:

else if (_token_type == 2) {
   require(IERC721(_token_addr).isApprovedForAll(msg.sender, address(this)),"011");
   transfer_token(_token_type, _token_addr, msg.sender, address(this), _total_tokens, _erc721_token_ids);
   //IERC721(_token_addr).setApprovalForAll(address(this), false);
}

Whether permission of management of ERC721 token granted to address(this) need to be withdrawed?

#### Recommendation:

If the answer is yes, uncomment L72 like below:

else if (_token_type == 2) {
   require(IERC721(_token_addr).isApprovedForAll(msg.sender, address(this)),"011");
   transfer_token(_token_type, _token_addr, msg.sender, address(this), _total_tokens, _erc721_token_ids);
   IERC721(_token_addr).setApprovalForAll(address(this), false);
}

------

<div style="page-break-after:always;"></div>

### UNP-08: Performs division before multiplication

| Type | Severity | Location |
| :----------- | :------------ | :----------------- |
| Optimization | Informational | redpacket.sol L210 |

#### Description:

claimed_tokens = random(seed, nonce) % SafeMath.mul(SafeMath.div(total_tokens, total_number - claimed_number), 2);

Performs division before multiplication may lose precision.

#### Recommendation:

Move multiplication before division like below:

claimed_tokens = random(seed, nonce) % SafeMath.div(SafeMath.mul(total_tokens, 2), total_number - claimed_number);

#### Alleviation:

Sujitech resolved in commit 229db2bffe9e2c38ea416164e3a1232347bca8b4.

------

<div style="page-break-after:always;"></div>

### UNP-09: Whether red packet with 0 amount make sense

| Type | Severity | Location |
| :----------- | :--------- | :----------------- |
| Optimization | Discussion | redpacket.sol L210 |

#### Description:

claimed_tokens = random(seed, nonce) % SafeMath.mul(SafeMath.div(total_tokens, total_number - claimed_number), 2);

It seems claimed_tokens have chance to be 0, whether red packet with 0 amount make sense?

#### Recommendation:

If not make sense, give a default min value 1 like below:

claimed_tokens = random(seed, nonce) % SafeMath.mul(SafeMath.div(total_tokens, total_number - claimed_number), 2);
if(claimed_tokens == 0) claimed_tokens = 1;

#### Alleviation:

Sujitech resolved in commit 229db2bffe9e2c38ea416164e3a1232347bca8b4.

------

<div style="page-break-after:always;"></div>

### UNP-10: Return value for claimed is not correct

| Type | Severity | Location |
| :----------- | :------- | :----------------- |
| Optimization | Minor | redpacket.sol L265 |

#### Description:

function check_availability(bytes32 id) public view returns (address token_address, uint balance, uint total, uint claimed, bool expired, bool ifclaimed) {
    RedPacket storage rp = redpacket_by_id[id];
    return (address(unbox(rp.packed2, 0, 160)), unbox(rp.packed1, 128, 80), unbox(rp.packed2, 232, 8), unbox(rp.packed2, 240, 8), now > unbox(rp.packed1, 208, 48), rp.claimed[msg.sender]);
}

`unbox(rp.packed2, 240, 8)` means token_type, not claimed.

#### Recommendation:

Correct it like below:

unbox(rp.packed2, 224, 8)

#### Alleviation:

Sujitech resolved in commit 229db2bffe9e2c38ea416164e3a1232347bca8b4.

------

<div style="page-break-after:always;"></div>

### UNP-11: Comparison to boolean constants

| Type | Severity | Location |
| :----------- | :------------ | :----------------- |
| Optimization | Informational | redpacket.sol L183 |

#### Description:

require (rp.claimed[recipient] == false, "005");

Boolean constants can be used directly and do not need to be compare to `true` or `false`.

#### Recommendation:

Use rp.claimed[recipient] directly like below:

require (rp.claimed[recipient], "005");

#### Alleviation:

Sujitech change their claimed validation solution.

------

<div style="page-break-after:always;"></div>

### UNP-12: Unlock solidity version & versions are different

| Type | Severity | Location |
| :----------- | :------------ | :----------------------------------------------------------- |
| Optimization | Informational | redpacket.sol, Migrations.sol, test_erc721_token.sol, test_token.sol L1 |

#### Description:

pragma solidity ^0.6;
pragma solidity >0.4.22;

Unlock solidity version & versions are different.

#### Recommendation:

Replace all versions with a specific version like below:

pragma solidity 0.6.12;

------

<div style="page-break-after:always;"></div>

### UNP-13: Public function that could be declared external

| Type | Severity | Location |
| :----------- | :------------ | :---------------------------------- |
| Optimization | Informational | redpacket.sol L57, L171, L262, L269 |

#### Description:

function create_red_packet (bytes32 _hash, uint _number, bool _ifrandom, uint _duration, 
                                bytes32 _seed, string memory _message, string memory _name,
                                uint _token_type, address _token_addr, uint _total_tokens,
                                uint256[] memory _erc721_token_ids) 
public payable

function claim(bytes32 id, string memory password, address _recipient, bytes32 validation) 
public returns (uint claimed)

function check_availability(bytes32 id) public view returns (address token_address, uint balance, uint total, uint claimed, bool expired, bool ifclaimed)

function check_erc721_token_ids(bytes32 id) public view returns (uint256[] memory erc721_token_ids) 

`public` functions that are never called by the contract should be declared `external` to save gas.

#### Recommendation:

Use the `external` attribute for functions never called from the contract like below:

function create_red_packet (bytes32 _hash, uint _number, bool _ifrandom, 
	uint _duration, bytes32 _seed, string memory _message, string memory _name, 
	uint _token_type, address _token_addr, uint _total_tokens, 
	uint256[] memory _erc721_token_ids) 
external payable

function claim(bytes32 id, string memory password, address _recipient, bytes32 validation) 
external returns (uint claimed)

function check_availability(bytes32 id) external view returns (address token_address, uint balance, uint total, uint claimed, bool expired, bool ifclaimed)

function check_erc721_token_ids(bytes32 id) external view returns (uint256[] memory erc721_token_ids) 

------

<div style="page-break-after:always;"></div>

### UNP-14: Nonexistent function name

| Type | Severity | Location |
| :----------- | :------- | :---------------------- |
| Optimization | Minor | redpacket.sol L276,L278 |

#### Description:

require(unpack(rp.packed1, 208, 48) < now, "012");

uint256 remaining_tokens = unpack(rp.redpack1, 128, 80);

Since there is no function named `unpack` in the contract, it is suspected that `unpack` lacks references or should use `unbox` function.

#### Recommendation:

Use the `unbox` function.

require(unbox(rp.packed1, 208, 48) < now, "012");

uint256 remaining_tokens = unbox(rp.redpack1, 128, 80);

#### **Alleviation**:

Sujitech resolved in commit de0becae17e5046f23c48ddad64763db74c62ccd

------

<div style="page-break-after:always;"></div>

### UNP-15: Error traversing the array

| Type | Severity | Location |
| :----------- | :------- | :----------------- |
| Optimization | Minor | redpacket.sol L293 |

#### Description:

uint256[] memory token_ids;
for (uint i = 0; i < rp.erc721_token_ids.length - 1; i++){
    if (rp.erc721_token_ids[i] != 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF) {
        token_ids[token_ids.length] = rp.erc721_token_ids[i];
        rp.erc721_token_ids[i] = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
    }
}

This loop will miss the last element which means the last token will be lost. And confirm the code on setting value to the dynamic length array. Missing a semicolon in Line 296.

#### Recommendation:

For reference:

uint256[] memory token_ids = new uint256[](remaining_tokens);
uint j = 0;
for (uint i = 0; i < rp.erc721_token_ids.length; i++){
    if (rp.erc721_token_ids[i] != 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF) {
        token_ids[j++] = rp.erc721_token_ids[i];
        rp.erc721_token_ids[i] = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;
    }
}

------

<div style="page-break-after:always;"></div>

### UNP-16: Too long digits

| Type | Severity | Location |
| :----------- | :------------ | :--------------------------- |
| Optimization | Informational | redpacket.sol L165 L294 L296 |

#### Description:

array[array.length - 1] = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;
uint256[] memory token_ids;
for (uint i = 0; i < rp.erc721_token_ids.length - 1; i++){
    if (rp.erc721_token_ids[i] != 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF) {
        token_ids[token_ids.length] = rp.erc721_token_ids[i];
        rp.erc721_token_ids[i] = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;
    }
}

“0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF” is too long to maintain.

#### Recommendation:

Create a constant value to store “0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF” like below:

unit256 constant mask = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;
array[array.length - 1] = mask;
uint256[] memory token_ids;
for (uint i = 0; i < rp.erc721_token_ids.length - 1; i++){
    if (rp.erc721_token_ids[i] != mask) {
        token_ids[token_ids.length] = rp.erc721_token_ids[i];
        rp.erc721_token_ids[i] = mask;
    }
}

#### Alleviation:

Sujitech resolved in commit 229db2bffe9e2c38ea416164e3a1232347bca8b4.

------

<div style="page-break-after:always;"></div>

### UNP-17: Redundant approve & Missing ‘;’

| Type | Severity | Location |
| :----------- | :------------ | :---------------------- |
| Optimization | Informational | redpacket.sol L286,L287 |

#### Description:

address token_address = unbox(rp.packed2, 0, 160)
IERC20(token_address).approve(msg.sender, remaining_tokens);
transfer_token(token_type, token_address, address(this),
            msg.sender, remaining_tokens, token_ids_holder);

L285 missed ‘;’. Besides, this approve is redundant, please reference to UNP-06.

#### Recommendation:

Add ‘;’ and remove approve like below:

address token_address = unbox(rp.packed2, 0, 160);
transfer_token(token_type, token_address, address(this),
            msg.sender, remaining_tokens, token_ids_holder);

#### Alleviation:

Sujitech resolved in commit de0becae17e5046f23c48ddad64763db74c62ccd.