ICON Smart Contract Bug

Smart contract auditing / peer review is an important one, when you deploy smart contracts on Ethereum. Time after time, we are presented with the scenarios how the non-audited smart contracts results in loss of funds which if in real world happens will result in complete turmoil. The recently discovered ICON bug is an example to reiterate this further. As a developer myself, issues in code a normal thing but luckily in traditional organizations the code should have gone through strenuous peer review and unit testing. This issue would have been avoided if the contract has under gone proper peer review / auditing (which i doubt didn't happen).

Let's deep dive into the issue in ICON smart contract issue. Below is the piece of code which caused the issue.

modifier onlyFromWallet {
require(msg.sender != walletAddress);
_;
}

A brief explanation about Modifiers here for the persons who don't know about them. Modifiers are used in solidity to sort of a prerequisite before any function is executed. Let's say for example, i want the function in contract to be executed only by the owner, then we can define the modifier and use it in function as below which is what they have done.

function enableTokenTransfer()
external
onlyFromWallet {
tokenTransfer = true;
TokenTransfer();
}

function disableTokenTransfer()
external
onlyFromWallet {
tokenTransfer = false;
TokenTransfer();
}

Note that the above two functions have the modifier "onlyFromWallet" defined. The main purpose of this is to make sure that only the contract owner can enable / disable the token transfer. But Check the condition in the modifier.

require(msg.sender != walletAddress);

The wallet address is the contract owner address and the msg.sender is the address of the person who is executing the smart contract function call. Note that "Not Equal to" condition here, which means the modifier requires the user not to be contract owner, which means anyone can enable / disable the token transfer.

The condition in the modifier should have been
require(msg.sender == walletAddress);

Worst Case Scenario:
Luckily this contract doesn't let people steal the money and it only allows people to consistently DDOS the contract and stop the token transfer at the expense of losing their ETH. The only way for mitigation i see is the ICON team start the migration of their ERC tokens to their main net as soon as possible. Until then any one with significant ETH can easily spam the contract and pause the transfer of tokens.