duytai/sguard

sguard's Issues

Understanding results of sGuard

I have tried running sGuard on the contract: https://github.com/smartbugs/smartbugs/blob/master/dataset/reentrancy/simple_dao.sol

sGuard produces the following output:

contract sGuard{
  function sub_uint256(uint256 a, uint256 b) internal pure returns (uint256) {
    assert(b <= a);
    return a - b;
  }

  bool internal locked_;
  constructor() internal {
    locked_ = false;
  }
  modifier nonReentrant_() {
    require(!locked_);
    locked_ = true;
    _;
    locked_ = false;
  }

  function add_uint256(uint256 a, uint256 b) internal pure returns (uint256) {
    uint256 c = a + b;
    assert(c >= a);
    return c;
  }
}
/*
 * @source: http://blockchain.unica.it/projects/ethereum-survey/attacks.html#simpledao
 * @author: -
 * @vulnerable_at_lines: 19
 */

pragma solidity ^0.4.2;

contract SimpleDAO  is sGuard {
  mapping (address => uint) public credit;

  function donate(address to) payable {
    credit[to] = add_uint256(credit[to], msg.value);
  }

   function withdraw(uint amount) nonReentrant_  {
    if (credit[msg.sender]>= amount) {
      // <yes> <report> REENTRANCY
      bool res = msg.sender.call.value(amount)(); /* <RE_ENT>, <RE_VUL> */
      credit[msg.sender] = sub_uint256(credit[msg.sender], amount);
    }
  }

  function queryCredit(address to) returns (uint){
    return credit[to];
  }
}

While the patched contract is safe from reentrancy vulnerabilities, I just wondered why sGuard does not add nonReentrant_ to donate function.

understanding the sGuard's output when muptiple contracts exist in a source file

Hi,

Could you give some help on understanding sGuard's output?

Suppose a below test contract file is given as input:

pragma solidity^0.4.26;
  
contract Fund {
  mapping(address => uint) balances;
  uint counter = 0;
  uint dontFixMe = 0;

  function main(uint x) public {
    if (counter < 100) {
      msg.sender.send(x + 1);
      counter += 1;
      dontFixMe ++;
    }
  }
}

contract Fund2 {
  mapping(address => uint) balances;
  uint counter = 0;
  uint dontFixMe = 0;

  function main(uint x) public {
    if (counter < 100) {
      msg.sender.send(x + 1);
      counter += 1;
      dontFixMe ++;
    }
  }
}


contract Fund3 {
  mapping(address => uint) balances;
  uint counter = 0;
  uint dontFixMe = 0;

  function main(uint x) public {
    if (counter < 100) {
      msg.sender.send(x + 1);
      counter += 1;
      dontFixMe ++;
    }
  }
}

When I designate Fund2 contract as main, the sGuard's output is the following:

contract sGuard{
  function add_uint256(uint256 a, uint256 b) internal pure returns (uint256) {
    uint256 c = a + b;
    assert(c >= a);
    return c;
  }

  bool internal locked_;
  constructor() internal {
    locked_ = false;
  }
  modifier nonReentrant_() {
    require(!locked_);
    locked_ = true;
    _;
    locked_ = false;
  }
}
pragma solidity^0.4.26;

contract Fund  is sGuard {
  mapping(address => uint) balances;
  uint counter = 0;
  uint dontFixMe = 0;

   function main(uint x) nonReentrant_  public {
    if (counter < 100) {
      msg.sender.send(x + 1);
      counter += 1;
      dontFixMe ++;
    }
  }
}

contract Fund2  is sGuard {
  mapping(address => uint) balances;
  uint counter = 0;
  uint dontFixMe = 0;

  function main(uint x) public {
    if (counter < 100) {
      msg.sender.send(add_uint256(x, 1));
      counter = add_uint256(counter, 1);
      dontFixMe ++;
    }
  }
}


contract Fund3  is sGuard {
  mapping(address => uint) balances;
  uint counter = 0;
  uint dontFixMe = 0;

  function main(uint x) public {
    if (counter < 100) {
      msg.sender.send(x + 1);
      counter += 1;
      dontFixMe ++;
    }
  }
}

My questions are the following:
[Q1] I wonder why sGuard added "nonReentrant_" modifier to the function in Fund (not the function in Fund2). As mentioned above, the specified main contract is Fund2.

[Q2] Independent from Q1, I wonder whether sGuard considers "send" instructions as sources of reentrancy vulnerabilities. To my knowledge, we cannot reenter to some functions using "send" due to gas fee restrictions on "send". However, sGuard seems to consider that the reentrancy is possible using "send".

How to sepeficy main contract ?

How can I specify a 'main' contract in a source file?

• Although a file may contain multiple contracts, only one contract can be designated as main, and the designated main contract is deployed.
• Or, could you point out what part should be changed to specify the name of a main contract?

Public the benchmark

I wonder whether the benchmark used in your paper will be public.
An auto-analysis is more attractive for us developers if its benchmark is public.
Thanks.

running sGuard on solc >= 0.8.0

Hi,

I am trying to run sGuard on solc >= 0.8.0.
It seems sGuard runs well on solc < 0.8.0, but it produces the following error message for the contract below.

How can I run sGuard successfully on solc >= 0.8.0 ?

• Error message

/home/ssb/sGuard/src/index.js:37
const { attributes: { name } } = children[children.length - 1]
                                                   ^

TypeError: Cannot read property 'length' of undefined
    at Object.<anonymous> (/home/ssb/sGuard/src/index.js:37:52)
    at Module._compile (internal/modules/cjs/loader.js:707:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:718:10)
    at Module.load (internal/modules/cjs/loader.js:605:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:544:12)
    at Function.Module._load (internal/modules/cjs/loader.js:536:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:760:12)
    at startup (internal/bootstrap/node.js:303:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:872:3)

• Tested contract

pragma solidity ^0.8.0;
//pragma solidity^0.4.26;

contract Fund {
  mapping(address => uint) balances;
  uint counter = 0;
  uint dontFixMe = 0;

  function main(uint x) public {
    if (counter < 100) {
      // msg.sender.send(x + 1);
      payable(msg.sender).send(x + 1);
      counter += 1;
      dontFixMe ++;
    }
  }
}

understanding behaviors of sGuard on fallback function

It seems sGuard does not add nonReentrant modifier to fallback functions. I wonder if there is any reason for it.

For example, given a test contract below:

contract Fund {
  mapping(address => uint) balances;
  uint counter = 0;
  uint dontFixMe = 0;

  function main(uint x) public {
    if (counter < 100) {
      msg.sender.send(x + 1);
      counter += 1;
      dontFixMe ++;
    }
  }

  function test () public {
    counter +=1;
  }

  function () public {
    counter +=1;
  }
}

sGuard outputs the following contract (it adds nonReentrant to test but not to the fallback):

contract Fund  is sGuard {
  mapping(address => uint) balances;
  uint counter = 0;
  uint dontFixMe = 0;

   function main(uint x) nonReentrant_  public {
    if (counter < 100) {
      msg.sender.send(add_uint256(x, 1));
      counter = add_uint256(counter, 1);
      dontFixMe ++;
    }
  }

   function test () nonReentrant_  public {
    counter = add_uint256(counter, 1);
  }

  function () public {
    counter = add_uint256(counter, 1);
  }
}

Many thanks for your reply in advance.