From 3cfc7691e10b7d3a9fcf0c8c8d645fe21425ffdd Mon Sep 17 00:00:00 2001 From: George Spasov Date: Wed, 27 Dec 2017 13:50:41 +0200 Subject: [PATCH] Comments --- erc20/ERC20Extended.sol | 16 +++++++++++----- erc20/ERC20Standard.sol | 4 ++-- erc20/data/ERC20ExtendedData.sol | 4 +++- erc20/data/ERC20StandardData.sol | 8 ++++++++ erc20/extensions/FreezableToken.sol | 5 +++-- ledger/Ledger.sol | 4 ++-- ledger/data/LedgerData.sol | 11 ++++++----- 7 files changed, 35 insertions(+), 17 deletions(-) diff --git a/erc20/ERC20Extended.sol b/erc20/ERC20Extended.sol index 344f016..4e33102 100644 --- a/erc20/ERC20Extended.sol +++ b/erc20/ERC20Extended.sol @@ -135,9 +135,15 @@ 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 } /** @@ -145,10 +151,10 @@ contract ERC20Extended is FreezableToken, PausableToken, BurnableToken, Mintable * @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); } diff --git a/erc20/ERC20Standard.sol b/erc20/ERC20Standard.sol index f10ffc0..79dfae9 100644 --- a/erc20/ERC20Standard.sol +++ b/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); diff --git a/erc20/data/ERC20ExtendedData.sol b/erc20/data/ERC20ExtendedData.sol index bf7d32e..72afc7f 100644 --- a/erc20/data/ERC20ExtendedData.sol +++ b/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]); } diff --git a/erc20/data/ERC20StandardData.sol b/erc20/data/ERC20StandardData.sol index 22a2397..2c5f3ca 100644 --- a/erc20/data/ERC20StandardData.sol +++ b/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; diff --git a/erc20/extensions/FreezableToken.sol b/erc20/extensions/FreezableToken.sol index 1cb851d..7132cd1 100644 --- a/erc20/extensions/FreezableToken.sol +++ b/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; diff --git a/ledger/Ledger.sol b/ledger/Ledger.sol index 27b827b..643d3f5 100644 --- a/ledger/Ledger.sol +++ b/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; } diff --git a/ledger/data/LedgerData.sol b/ledger/data/LedgerData.sol index 048850a..e17a111 100644 --- a/ledger/data/LedgerData.sol +++ b/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; }