The Incent Coffee smart contract is currently being used to test out the Waves <-> Ethereum block swapper at https://blockswap.incentloyalty.com/. A version of the contract has been deployed to 0x2c875e5ea4706b1978a41b59edf2d3af31d60e70. This contract was written by Bok Consulting Pty Ltd in collaboration with Incent Loyalty Pty Ltd.
I have conducted a security audit on the deployed smart contract, resulting in three recommendation to improve on minor security issues which are:
balances[_to]
should be checked for overflows in thetransfer(...)
method.balances[_to]
should be checked for overflows in thetransferFrom(...)
method.- The statements in
transferFrom(...)
should be reordered for additional security
The revised smart contract is listed below the original smart contract.
Other notes:
- This smart contract does not have a fallback
()
function that is marked payable. Ethers cannot normally be accidentally sent to this smart contract (except from theselfdestruct
opcode. - Tokens from other contract may accidentally be sent to this contract. There is no means to transfer these accidentally sent tokens back out from this contract. It would be easy enough to write a method for the owner of this contract to transfer out any ERC20-compliant tokens so it can be returned to the sender, but keeping the code of this contract simple is of more importance.
- There are no external calls to worry about in this contract.
- There is no integer division to worry about in this contract.
- There is no division by zero to worry about in this contract.
- There could be integer overflow errors in this contract, but it will be impossible when the totalSupply is way way below 2^256 / 2 .
- There is no potential reentrancy problems as no calls to external addresses or contracts are done.
- There is no timestamp dependency to worry about in this contract.
- There is no circuit breaker in this contract. In the worst case the Waves <-> Ethereum block swapper can be halted, and the contract upgraded by issuing a new contract at a different address with the verified token balances transferred over.
- There are no delayed contract actions or rate limiting features in this contract. This can be implemented in the Waves <-> Ethereum block swapper if necessary.
- A bug bounty program is recommended for this contract.
- The reordering of statements in
transferFrom(...)
is just in case a transaction runs out of gas, but the execution completes. For an example of this case, see https://github.com/bokkypoobah/TheDAOData/blob/master/ExtraBalanceReconciliation.md, where the transaction Tx 0xe290b5d4…. apparently runs out of gas but completes it’s execution.
Other references: Ethereum Contract Security Techniques and Tips.
The Current Contract
Following is the current contract at 0x2c875e5ea4706b1978a41b59edf2d3af31d60e70, with my additional comments prefixed with // CHECK:
, and my recommendations prefixed with // RECOMMENDATION:
:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221 222 |
pragma solidity ^0.4.8; // ---------------------------------------------------------------------------------------------- // A collaboration between Incent and Bok :) // Enjoy. (c) Incent Loyalty Pty Ltd and Bok Consulting Pty Ltd 2017. The MIT Licence. // ---------------------------------------------------------------------------------------------- // CHECK: Nothing exciting here. // CHECK: Consistent with https://github.com/ConsenSys/Tokens/blob/master/Token_Contracts/contracts/HumanStandardToken.sol // Contract configuration contract TokenConfig { string public constant symbol = "INCOF"; string public constant name = "Incent Coffee Token"; uint8 public constant decimals = 0; // 0 decimal places, the same as tokens on Wave uint256 _totalSupply = 824; } // CHECK: Nothing exciting here. // CHECK: Confirmed that the functions and events are consistent with https://github.com/ethereum/EIPs/issues/20 // ERC Token Standard #20 Interface // https://github.com/ethereum/EIPs/issues/20 contract ERC20Interface { // Get the total token supply function totalSupply() constant returns (uint256 totalSupply); // Get the account balance of another account with address _owner function balanceOf(address _owner) constant returns (uint256 balance); // Send _value amount of tokens to address _to function transfer(address _to, uint256 _value) returns (bool success); // Send _value amount of tokens from address _from to address _to function transferFrom(address _from, address _to, uint256 _value) returns (bool success); // Allow _spender to withdraw from your account, multiple times, up to the _value amount. // If this function is called again it overwrites the current allowance with _value. // this function is required for some DEX functionality function approve(address _spender, uint256 _value) returns (bool success); // Returns the amount which _spender is still allowed to withdraw from _owner function allowance(address _owner, address _spender) constant returns (uint256 remaining); // Triggered when tokens are transferred. event Transfer(address indexed _from, address indexed _to, uint256 _value); // Triggered whenever approve(address _spender, uint256 _value) is called. event Approval(address indexed _owner, address indexed _spender, uint256 _value); } contract IncentCoffeeToken is ERC20Interface, TokenConfig { // CHECK: OK // Owner of this contract address public owner; // CHECK: OK // Balances for each account mapping(address => uint256) balances; // CHECK: OK // Owner of account approves the transfer of an amount to another account mapping(address => mapping (address => uint256)) allowed; // CHECK: OK // Functions with this modifier can only be executed by the owner modifier onlyOwner() { if (msg.sender != owner) { throw; } _; } // CHECK: 1. Owner deploys this contract, so only the owner executes this constructor // CHECK: 2. Fixed supply specified in TokenConfig._totalSupply // CHECK: 3. Owner's balance starts off as the _totalSupply // Constructor function IncentCoffeeToken() { owner = msg.sender; balances[owner] = _totalSupply; } // CHECK: 1. Anyone can call this function // CHECK: 2. This function is a constant function // CHECK: 3. The return type of this function is consistent with the _totalSupply member // CHECK: 4. The function signature is consistent with https://github.com/ethereum/EIPs/issues/20 function totalSupply() constant returns (uint256 totalSupply) { totalSupply = _totalSupply; } // CHECK: 1. Anyone can call this function // CHECK: 2. This function is a constant function // CHECK: 3. The return type of this function is consistent with the balances data structure // CHECK: 4. The input parameter is used with the balances data structure consistently // CHECK: 5. The function signature is consistent with https://github.com/ethereum/EIPs/issues/20 // What is the balance of a particular account? function balanceOf(address _owner) constant returns (uint256 balance) { return balances[_owner]; } // CHECK: 1. This is the 1st of the 2 risky parts of this smart contract. Additional care is required here // CHECK: 2. Anyone can call this function // CHECK: 3. This function is NOT a constant function. Anyone can change the state of this contract's // balances data structure // CHECK: 4. Even non-token holders can call this function but their balance will be 0 and will fail the first condition // CHECK: 5. The function signature is consistent with https://github.com/ethereum/EIPs/issues/20 // CHECK: 6. If the senders balance is half or more than 2^256 and _amount is half or less than 2^256, the senders balance // will be reduced appropriately, but the account receiving the balance could be overflowed. This will not be // a problem here as _totalSupply and anyone's balance will be way way below 2^256 / 2 // CHECK: 7. The account executing this function could specify a low gas amount and receiving account // may fail to receive the transferred amount. But the account executing this function // will have it's balance reduced. This does not affect the security of this contract // CHECK: 8. The account executing this function could specify a low gas amount and the Transfer // event may fail to get logged. But this does not affect the security of this contract // CHECK: 9. There is a check that the sender has more (or equal) the amount being transferred, and that // the amount being transferred is more than 0. Integer underflow are prevented // CHECK: 10. This function will return false if the transfer cannot occur, and the moveToWaves(...) function // will not generate the WavesTransfer event // RECOMMENDATION: 1. Add a check that balances[_to] + amount > balances[_to] // // Transfer the balance from owner's account to another account function transfer(address _to, uint256 _amount) returns (bool success) { if (balances[msg.sender] >= _amount && _amount > 0) { balances[msg.sender] -= _amount; balances[_to] += _amount; Transfer(msg.sender, _to, _amount); return true; } else { return false; } } // CHECK: 1. This is the 2nd of the 2 risky parts of this smart contract. Additional care is required here // CHECK: 2. Anyone can call this function // CHECK: 3. This function is NOT a constant function. Anyone can change the state of this contract's // balances and allowed data structures // CHECK: 4. Even non-token holders can call this function but their balance will be 0 and will fail the first condition // CHECK: 5. The function signature is consistent with https://github.com/ethereum/EIPs/issues/20 // CHECK: 6. If the _from balance is half or more than 2^256 and _amount is half or less than 2^256, the _from balance // will be reduced appropriately, but the _to balance could be overflowed. This will not be // a problem here as _totalSupply and anyone's balance will be way way below 2^256 / 2 // CHECK: 7. The account executing this function could specify a low gas amount and balances[_to] will be increase // while balances[_from] fails to execute. RECOMMENDATION 1 below will be to alter the order of statements // so that the balances[_from] is deducted from before balances[_to] is increased // CHECK: 8. The account executing this function could specify a low gas amount and the Transfer // event may fail to get logged. But this does not affect the security of this contract // CHECK: 9. There is a check that balance[_from] has more (or equal) the amount being transferred, and that // the amount being transferred is more than 0. Integer underflow are prevented // RECOMMENDATION: 2. Add a check that balances[_to] + amount > balances[_to] // RECOMMENDATION: 3. Move the 'balances[_to] += _amount;' statement after the // 'allowed[_from][msg.sender] -= _amount;' statement // // Send _value amount of tokens from address _from to address _to // The transferFrom method is used for a withdraw workflow, allowing contracts to send // tokens on your behalf, for example to "deposit" to a contract address and/or to charge // fees in sub-currencies; the command should fail unless the _from account has // deliberately authorized the sender of the message via some mechanism; we propose // these standardized APIs for approval: function transferFrom( address _from, address _to, uint256 _amount ) returns (bool success) { if (balances[_from] >= _amount && allowed[_from][msg.sender] >= _amount && _amount > 0) { balances[_to] += _amount; balances[_from] -= _amount; allowed[_from][msg.sender] -= _amount; Transfer(_from, _to, _amount); return true; } else { return false; } } // CHECK: 1. Anyone can call this function // CHECK: 2. This function is NOT a constant function. Anyone can change the state of this contract's // allowed data structure // CHECK: 3. Even non-token holders can call this function and specify a spender's limit // CHECK: 4. This function always returns true, and this is consistent with the return type // CHECK: 5. The account executing this function could specify a low gas amount and the Approval // event may fail to get logged. But this does not affect the security of this contract // CHECK: 6. The function signature is consistent with https://github.com/ethereum/EIPs/issues/20 // Allow _spender to withdraw from your account, multiple times, up to the _value amount. // If this function is called again it overwrites the current allowance with _value. function approve(address _spender, uint256 _amount) returns (bool success) { allowed[msg.sender][_spender] = _amount; Approval(msg.sender, _spender, _amount); return true; } // CHECK: 1. Anyone can call this function // CHECK: 2. This function is a constant function // CHECK: 3. The return type of this function is consistent with the allowed data structure // CHECK: 4. The input parameters are used with the allowed data structure consistently // CHECK: 5. The function signature is consistent with https://github.com/ethereum/EIPs/issues/20 function allowance(address _owner, address _spender) constant returns (uint256 remaining) { return allowed[_owner][_spender]; } } contract WavesEthereumSwap is IncentCoffeeToken { // CHECK: OK event WavesTransfer(address indexed _from, string wavesAddress, uint256 amount); // CHECK: 1. Anyone can call this function // CHECK: 2. This function is NOT a constant function. Anyone can change the state of this contract // CHECK: 3. Relying on the check in the super.transfer(...) function // CHECK: 4. The input parameter wavesAddress could be null or very long. This does not change the // state of this contract, but is only used to generate the event log. // CHECK: 5. The account executing this function could specify a low gas amount and the WavesTransfer // event may fail to get logged. But the account will lose out in getting their tokens // transferred to the Waves blockchain // CHECK: 6. If the account does not have sufficient tokens to transfer, an error will be thrown and // the WavesTransfer event will not be logged. The token will not be transferred to the // Waves blockchain // CHECK: 7. The amount parameter is consistent with the super.transfer(...) function // CHECK: 8. The amount parameter is consistent with the WavesTransfer(...) event amount parameter function moveToWaves(string wavesAddress, uint256 amount) { if (!transfer(owner, amount)) throw; WavesTransfer(msg.sender, wavesAddress, amount); } } |
The Revised Contract
Here is the revised smart contract that implements the 3 recommendations:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 |
pragma solidity ^0.4.8; // ---------------------------------------------------------------------------------------------- // A collaboration between Incent and Bok :) // Enjoy. (c) Incent Loyalty Pty Ltd and Bok Consulting Pty Ltd 2017. The MIT Licence. // ---------------------------------------------------------------------------------------------- // Contract configuration contract TokenConfig { string public constant symbol = "INCOF"; string public constant name = "Incent Coffee Token"; uint8 public constant decimals = 0; // 0 decimal places, the same as tokens on Wave uint256 _totalSupply = 824; } // ERC Token Standard #20 Interface // https://github.com/ethereum/EIPs/issues/20 contract ERC20Interface { // Get the total token supply function totalSupply() constant returns (uint256 totalSupply); // Get the account balance of another account with address _owner function balanceOf(address _owner) constant returns (uint256 balance); // Send _value amount of tokens to address _to function transfer(address _to, uint256 _value) returns (bool success); // Send _value amount of tokens from address _from to address _to function transferFrom(address _from, address _to, uint256 _value) returns (bool success); // Allow _spender to withdraw from your account, multiple times, up to the _value amount. // If this function is called again it overwrites the current allowance with _value. // this function is required for some DEX functionality function approve(address _spender, uint256 _value) returns (bool success); // Returns the amount which _spender is still allowed to withdraw from _owner function allowance(address _owner, address _spender) constant returns (uint256 remaining); // Triggered when tokens are transferred. event Transfer(address indexed _from, address indexed _to, uint256 _value); // Triggered whenever approve(address _spender, uint256 _value) is called. event Approval(address indexed _owner, address indexed _spender, uint256 _value); } contract IncentCoffeeToken is ERC20Interface, TokenConfig { // Owner of this contract address public owner; // Balances for each account mapping(address => uint256) balances; // Owner of account approves the transfer of an amount to another account mapping(address => mapping (address => uint256)) allowed; // Functions with this modifier can only be executed by the owner modifier onlyOwner() { if (msg.sender != owner) { throw; } _; } // Constructor function IncentCoffeeToken() { owner = msg.sender; balances[owner] = _totalSupply; } function totalSupply() constant returns (uint256 totalSupply) { totalSupply = _totalSupply; } // What is the balance of a particular account? function balanceOf(address _owner) constant returns (uint256 balance) { return balances[_owner]; } // Transfer the balance from owner's account to another account function transfer(address _to, uint256 _amount) returns (bool success) { if (balances[msg.sender] >= _amount && _amount > 0 && balances[_to] + _amount > balances[_to]) { balances[msg.sender] -= _amount; balances[_to] += _amount; Transfer(msg.sender, _to, _amount); return true; } else { return false; } } // Send _value amount of tokens from address _from to address _to // The transferFrom method is used for a withdraw workflow, allowing contracts to send // tokens on your behalf, for example to "deposit" to a contract address and/or to charge // fees in sub-currencies; the command should fail unless the _from account has // deliberately authorized the sender of the message via some mechanism; we propose // these standardized APIs for approval: function transferFrom( address _from, address _to, uint256 _amount ) returns (bool success) { if (balances[_from] >= _amount && allowed[_from][msg.sender] >= _amount && _amount > 0 && balances[_to] + _amount > balances[_to]) { balances[_from] -= _amount; allowed[_from][msg.sender] -= _amount; balances[_to] += _amount; Transfer(_from, _to, _amount); return true; } else { return false; } } // Allow _spender to withdraw from your account, multiple times, up to the _value amount. // If this function is called again it overwrites the current allowance with _value. function approve(address _spender, uint256 _amount) returns (bool success) { allowed[msg.sender][_spender] = _amount; Approval(msg.sender, _spender, _amount); return true; } function allowance(address _owner, address _spender) constant returns (uint256 remaining) { return allowed[_owner][_spender]; } } contract WavesEthereumSwap is IncentCoffeeToken { event WavesTransfer(address indexed _from, string wavesAddress, uint256 amount); function moveToWaves(string wavesAddress, uint256 amount) { if (!transfer(owner, amount)) throw; WavesTransfer(msg.sender, wavesAddress, amount); } } |