Audit Feedback

erc20/ERC20Extended.sol

@@ -135,20 +135,26 @@ contract ERC20Extended is FreezableToken, PausableToken, BurnableToken, Mintable
     * @dev Send Ether to buy tokens at the current token sell price.
     * @notice Throws on failure.
     */
-    function buy() payable whenNotPaused public {
-        uint256 amount = msg.value.div(sellPrice);
-        _transfer(this, msg.sender, amount);
+    function buy() payable whenNotPaused public { // AUDIT: It's best to keep the returns (bool success) that you have started
+
+        /* AUDIT: There might be a case for setting minimal contribution. 
+            In general try different cases of the sell price and wei sent. There might be some of them that break the math as there are no floating points.
+            The floating point problem is generally resolved by adding decimals and/or minimal contribution.
+            If you are basing some logic on any assumption like 'The price will always be > than 1 eth or the contribution will always be > than the sellPrice' 
+            it is probably good idea to include them in the code */
+        uint256 amount = msg.value.div(sellPrice); // AUDIT: There might be a reminder you can get it by modulus division and transfer it back
+        _transfer(this, msg.sender, amount); // AUDIT: Just in case wrap this in assert
     }
 
     /**
     * @dev Sell `_amount` tokens at the current buy price.
     * @param _amount The amount to sell.
     * @notice Throws on failure.
     */
-    function sell(uint256 _amount) whenNotPaused public {
+    function sell(uint256 _amount) whenNotPaused public { // AUDIT: It's best to keep the returns (bool success) that you have started
         uint256 toBeTransferred = _amount.mul(buyPrice);
         require(this.balance >= toBeTransferred);
-        require(_transfer(msg.sender, this, _amount));
+        require(_transfer(msg.sender, this, _amount)); // AUDIT: It's a bit better to use assert for success validations and keep require for user input validation
         msg.sender.transfer(toBeTransferred);
     }
 

erc20/ERC20Standard.sol

@@ -82,7 +82,7 @@ contract ERC20Standard {
     * @param _value The amount to be transferred.
     * @return success True if the transfer was successful, or throws.
     */
-    function transfer(address _to, uint256 _value) public returns (bool success) {
+    function transfer(address _to, uint256 _value) public returns (bool success) { // AUDIT: In our case I would put requirement for greater than 0 value as someone might try to fill the ledger up with useless 0 value transactions
         return _transfer(msg.sender, _to, _value);
     }
 
@@ -93,7 +93,7 @@ contract ERC20Standard {
      * @param _value The amount to send.
      * @return success True if the transfer was successful, or throws.
      */    
-    function transferFrom(address _from, address _to, uint256 _value) public returns (bool success) {
+    function transferFrom(address _from, address _to, uint256 _value) public returns (bool success) { // AUDIT: Same as above
         uint256 allowed = dataStorage.getAllowance(_from, msg.sender);
         require(allowed >= _value);
 

erc20/data/ERC20ExtendedData.sol

@@ -9,7 +9,9 @@ import "./ERC20StandardData.sol";
 contract ERC20ExtendedData is ERC20StandardData {
     mapping(address => bool) private frozenAccounts;
 
-    function getFrozenAccount(address _target) public view returns (bool isFrozen, uint256 amount) {
+    function getFrozenAccount(address _target) public view returns (bool isFrozen, uint256 amount) { /* AUDIT:  There might be a case for returning only if the account is frozen (thus might need renaming)
+                                                                                                                as there is other constant methods that will return the balance if it is needed. 
+                                                                                                                This will reduce the complexity in the imlementation */
         return (frozenAccounts[_target], balances[_target]);
     }
 

erc20/data/ERC20StandardData.sol

@@ -8,6 +8,14 @@ import "../../modifiers/FromContract.sol";
  */
 contract ERC20StandardData is FromContract {
 
+
+    /*
+
+    AUDIT: In general when using public blockchain no data is ever private regardless of the modifiers. 
+    Thus the general rule is to basically keep almost all of your fields (IMPORTANT: not methods) public 
+    as it atleast generates for you getter methods and allows you to write less code. Therefore you might opt for
+    making the three fields public and removing the getX methods and leaving the setters only
+    */
     mapping(address => uint256) internal balances;
 
     mapping(address => mapping (address => uint256)) private allowed;

erc20/extensions/FreezableToken.sol

@@ -48,12 +48,13 @@ contract FreezableToken is ERC20Standard, Ownable {
      * @return amount Balance of the account.
      */
     function isAccountFrozen(address _target) public view returns (bool isFrozen, uint256 amount) {
-        return dataStorage.getFrozenAccount(_target);
+        return dataStorage.getFrozenAccount(_target); // AUDIT: If you remove the amount from the dataStorage method you might use dataStorage.getBalance() to get the second param and still keep it all constant
     }
 
+    /* AUDIT: It might be a bit better to do this logic with a modifier onlyNotFrozen */
     function _transfer(address _from, address _to, uint256 _value) internal returns (bool success) {
         bool isFromFrozen;
-        (isFromFrozen,) = dataStorage.getFrozenAccount(_from);
+        (isFromFrozen,) = dataStorage.getFrozenAccount(_from); // AUDIT: If you remove the amount from the dataStorage method the three rows can be done in 1 here and below
         require(!isFromFrozen);
 
         bool isToFrozen;

ledger/Ledger.sol

@@ -13,12 +13,12 @@ contract Ledger is FromContract, Destroyable {
 
     LedgerDataStorage internal ledgerDataStorage;
 
-    function Ledger (address ledgerDataStorageAddress) public {
+    function Ledger (address ledgerDataStorageAddress) public { // AUDIT: require for ledgerDataStorageAddress to be non-zero
         ledgerDataStorage = LedgerDataStorage(ledgerDataStorageAddress);
     }
 
     function addTransaction(address _from, address _to, uint _tokens) public fromContract returns (bool) {
-        ledgerDataStorage.addTransaction(_from, _to, _tokens);
+        ledgerDataStorage.addTransaction(_from, _to, _tokens); // AUDIT: Wrap this in assert()
         return true;
     }
 

ledger/data/LedgerData.sol

@@ -8,14 +8,15 @@ contract LedgerData is FromContract {
     using SafeMath for uint256;
 
     struct Transaction {
-        uint id;
+        uint id; // AUDIT: Save storage space by not using this field as you already have this data in the key
         address from;
         address to;
-        uint tokens;
+        uint tokens; // AUDIT: Keep using uint256 as you've started with it in the ERC20 things
     }
 
-    mapping(uint => Transaction) private transactions;
-    uint private transactionsLength = 0;
+    mapping(uint => Transaction) private transactions; // AUDIT: Make this public and you wont need the method for getting Transaction as it is auto generated
+
+    uint private transactionsLength = 0; // AUDIT: Make this public and you wont need the method for getting transaction length as it is auto generated
 
     function getTransaction(uint _index) public view returns (uint, address, address, uint) {
         return (transactions[_index].id, transactions[_index].from, transactions[_index].to, transactions[_index].tokens);
@@ -35,7 +36,7 @@ contract LedgerData is FromContract {
         return transactionsLength;
     }
 
-    function setTransactionsLength(uint _value) internal returns (bool) {
+    function setTransactionsLength(uint _value) internal returns (bool) { // AUDIT: Making the transactionsLength public will render this method obsolete
         transactionsLength = _value;
         return true;
     }