---

title: CertiK Auditor Note For IPI

tags : audit-note

---
CertiK Auditor Note For IPI
===

<!-- Auditor note should be submitted daily. Use 10 mins per day to finish it and push to the project git repo through the hackmd embeded plugin: https://hackmd.io/c/tutorials/%2Fs%2Flink-with-github -->

###### tags: `Note` `Audit`

:::info
- **Project:** IPI
- **Date:** Dec 9, 2020
- **Auditor:** [e-mail], [e-mail]
- **Reference:** [ipi-token](https://github.com/InvestAndPay/ipi-token)

:::

:mag: New Findings
---
<!-- List all findings you think worthy note in the process of the audit. Not necessarily list here all vulnerabilities found in the project. List -->

| Finding Index | Finding Type | Finding Name | Description | Feedback |
| ------------- | ------------- | ------------ | ------------- | -------- |
| 1 | Security risk | `tx.origin` usage | We recommend to use `msg.sender` instead of `tx.orgin` | We use one governace address to manage all contracts. If we check msg.sender , our govenance address cannot call all the onlyOwner functions in all contracts. We will use the private key of governance to create the Owners contract. Then use Owners to create BillManager and Token. |

:closed_book: 项目描述
--
该项目是在[ERC1155](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1155.md)协议上基础上创建的票据管理类型项目，主要功能是完成票据的创建、销毁、上传、校验以及票据对应代币的发行和赎回等。
## 项目结构
项目是MultiOwnable通过BillManager来创建Bill并和IPIToken进行交互来完成功能。
## 发现问题
在MultiOwnable.sol中发现存在以下代码。

    modifier onlyMainOwner() {
        require(
            tx.origin == ownersContract.mainOwner(),
            "MultiOwnable: caller is not the main owner");
        _;
    }

项目方目的是使用owner合约来控制整个项目流程，所以利用`tx.origin`来鉴权。`tx.origin`用来做鉴权的话，是可能会被钓鱼攻击的。详情参考[Recommendation](#Recommendation)

## 项目沟通
1. 在第一个初审报告版本中，提及到`tx.origin`的使用风险问题，项目方不愿意改，理由是管理员方便同时管理Bill和代币的发行和赎回等，并充分考虑了`tx.origin`的安全性。
然后写了以下攻击合约给项目方：

    pragma solidity ^0.6.12;

    interface MultiOwnable {
        function setOwnersContract(Owners _owners) external onlyOwners;
    }
    interface BillManager {
        function setOwnersContract(Owners _owners) external onlyOwners;
    }
    interface CNYToken {
        function setOwnersContract(Owners _owners) external onlyOwners;
    }

    contract AttackContract {
        address public hackAddress = 0xagawegwg...;
        address public yourBillManagerAddress = 0x...;
        address public yourTokenAddress = 0x...;
        constructor() public {

        }

        function mainOwner() external view returns (address) {
            return hackAddress;
        }

        function isOwner(address who) external view returns (bool) {
            //所有isOwner的方法均失效
            return true;
        }

        function receive() external payable {
            //更改BillManager里面权限合约地址。
            BillManager(yourBillManagerAddress).setOwnersContract(address(this));
            //更改CNYToken合约权限合约地址。
            CNYToken(yourTokenAddress).setOwnersContract(address(this));
        }
    }

2. 项目方重构了代码，移除了`setOwnersContract()`方法。仍然没有尝试用`msg.sender`替代`tx.origin`。
3. 然后发送[Recommendation](#Recommendation)文件给项目方。
4. 项目方修改代码。直接替换为`msg.sender`。通过`addOwner()`把BillManager地址和IPIToken地址互相添加为owner完成项目需求。

### Recommendation

根据[solidity官方文档](https://docs.soliditylang.org/en/v0.6.12/units-and-global-variables.html?highlight=tx.origin#block-and-transaction-properties)说明，

- **tx.origin**是类型为*address*的全局变量，为**交易发起者（完全的调用链）**，为用户账户地址。

- **msg.sender**是类型为*address*的全局变量，为**消息发送方（当前调用）**，可以为合约或者用户地址。

对于合约来说，**tx.origin**是过程不透明的，在一个简单的调用链中，A -> B -> C -> D，此时**tx.origin**为A，而**msg.sender**为C。所以如果D使用**tx.origin**进行身份认证的话，如果B或者C为钓鱼合约，一旦A调用，则D即可以被攻击。

solidity官方文档不推荐采用[tx.origin用来鉴权](https://solidity-cn.readthedocs.io/zh/develop/security-considerations.html?highlight=tx.origin#tx-origin)。

**tx.origin**在特定环境下用来鉴权，例如通过判断**tx.origin**是否与**msg.sender**相同来判断调用者是否为合约。

考虑到IPI目前结构已经基本完成的情况下，大规模的代码改动需要的工作量和时间成本是很大的。所以我们通过综合考虑，建议增加一个Controller作为公共入口，将IPIToken和BillManager的方法整合在其中。具体步骤如下（仅供参考，请具体根据实际需求修改并自行测试）：

1. 修改**MultiOwnable.sol**继承**Owners**,修改如下：

   contract MultiOwnable is Owners{
     	modifier onlyOwners() {
           require(msg.sender == this.mainOwner() || this.isOwner(msg.sender), "MultiOwnable: caller is not the owner members");
           _;
       }
   }

2. 创建Controller文件，继承**MultiOwnable**。增加BillManager和IPIToken的全局变量。添加具体方法类似如下：

   pragma solidity 0.6.12;
   
   import "./Common/MultiOwnable.sol";
   import "./BillManager.sol";
   contract Controller is MultiOwnable{
       
       BillManager public immutable billManager;
       
       constructor(BillManager _manager,IPIToken _token) public{
           require(address(_manager) != address(0),"_manager is a zero address");
           billManager = _manager;
       }
       
       function createBill(
           uint256 _id,
           uint256 _billAmount,
           string memory _drawer,
           string memory _draweeBankName,
           string memory _accepter,
           string memory _accepterBankName,
           bytes8 _draweeDate,
           bytes8 _expireDate
       ) external onlyOwners returns (Bill) {
           billManager.createBill(_id, _billAmount, _drawer, _draweeBankName, _accepter, _accepterBankName, _draweeDate, _expireDate);
       }
   }

3. 在BillManager和IPIToken合约上添加Controller合约地址为owner。

4. 在Controller中添加用户为owner，该用户即可调用**createBill**等方法。