Solidity Hack
Secureum summary notes
1-Solidity versions: Using very old versions of Solidity prevents benefits of bug fixes and newer security checks.
Solution : Use 0.8.4 instead of 0.8.12
2-Unlocked pragma: Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the pragma (for e.g. by not using ^ in pragma solidity 0.5.10) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs.
Solution: If you deploy with 0.8.4, you must compile with 0.8.4
3-Multiple Solidity pragma: It is better to use one Solidity compiler version across all contracts instead of different versions with different bugs and security checks.
Solution : Use 0.8.4 instead of “>=0.8.4 <=0.8.12”
4- Incorrect access control: Contract functions executing critical logic should have appropriate access control enforced via address checks (e.g. owner, controller etc.) typically in modifiers.
Solutions:
a- An onlyOwner modifier is defined but not used, allowing anyone to become the owner
b- Rubixi allows anyone to become owner
//Sets creator
function DynamicPyramid() {
creator = msg.sender;
}
modifier onlyowner {
if (msg.sender == creator) _
}
c- A contract with a changeOwner function does not label it as private and therefore allows anyone to become the contract owner.
5. Unprotected withdraw function: Unprotected (external/public) function calls sending Ether/tokens to user-controlled addresses may allow users to withdraw unauthorized funds.
Solution: Due to missing or insufficient access controls, malicious parties can withdraw some or all Ether from the contract account.
6- Unprotected call to selfdestruct: A user/attacker can intentionally kill the contract.
Solution:Protect access to such functions.
7- Modifier side-effects: Modifiers should only implement checks and not make state changes.
Solution:The modifier should be simple and shouldn’t call external contracts
8-Incorrect modifier: If a modifier does not execute _ or revert, the function using that modifier will return the default value causing unexpected behavior.
Solution: All paths in a modifier should execute _ or revert
modifier goodModif(){
if(..)
{ _; }
else
{ revert(“ERROR”);
// or _; }
}
function get() goodModif returns(address){}
9- Constructor names: Before solc 0.4.22, constructor names had to be the same name as the contract class containing it.
10-Void constructor: Calls to base contract constructors that are unimplemented leads to misplaced assumptions.
Solution: Remove the constructor call.
11-Implicit constructor callValue check: The creation code of a contract that does not define a constructor but has a base that does, did not revert for calls with non-zero callValue when such a constructor was not explicitly payable. This is due to a compiler bug introduced in v0.4.5 and fixed in v0.6.8.
12-Controlled delegatecall: delegatecall() or callcode() to an address controlled by the user allows execution of malicious contracts in the context of the caller’s state.
Solution: Ensure trusted destination addresses for such calls.
13-Reentrancy vulnerabilities: Untrusted external contract calls could callback leading to unexpected results such as multiple withdrawals or out-of-order events.
Solution:Use check-effects-interactions pattern or reentrancy guards
a-
credit[msg.sender]>= amount)
credit[msg.sender]-=amount;
require(msg.sender.call.value(amount)());
15-Avoid transfer()/send() as reentrancy mitigations: Although transfer() and send() have been recommended as a security best-practice to prevent reentrancy attacks because they only forward 2300 gas, the gas repricing of opcodes may break deployed contracts. Use call() instead, without hardcoded gas limits along with checks-effects-interactions pattern or reentrancy guards for reentrancy protection.
Solution: Use call() function with reentrancy protection
16- Private on-chain data: Marking variables private does not mean that they cannot be read on-chain.
Solution:Any private data should either be stored off-chain, or carefully encrypted.
17-Weak PRNG: PRNG relying on block.timestamp, now or blockhash can be influenced by miners to some extent and should be avoided.
Solution:Use of blockhash, block.difficulty and other fields is also insecure, as they're controlled by the miner.
18-Block values as time proxies: block.timestamp and block.number are not good proxies (i.e. representations, not to be confused with smart contract proxy/implementation pattern) for time because of issues with synchronization, miner manipulation and changing block times.
Solution:use oracles
19-Integer overflow/underflow: Not using OpenZeppelin’s SafeMath (or similar libraries) that check for overflows/underflows may lead to vulnerabilities or unexpected behavior if user/attacker can control the integer operands of such arithmetic operations.
Solution:use Safemath
20-Divide before multiply: Performing multiplication before division is generally better to avoid loss of precision because Solidity integer division might truncate.
Solution:it’s usually a good idea to re-arrange arithmetic to perform multiplication before division, unless the limit of a smaller type makes this dangerous.
22-ERC20 approve() race condition: Use safeIncreaseAllowance() and safeDecreaseAllowance() from OpenZeppelin’s SafeERC20 implementation to prevent race conditions from manipulating the allowance amounts
24-ERC20 transfer() does not return boolean: Contracts compiled with solc >= 0.4.22 interacting with such functions will revert.
Solution:Use OpenZeppelin’s SafeERC20 wrappers
25-Incorrect return values for ERC721 ownerOf(): Contracts compiled with solc >= 0.4.22 interacting with ERC721 ownerOf() that returns a bool instead of address type will revert.
Solution:Use OpenZeppelin’s ERC721 contracts.
26-Unexpected Ether and this.balance: A contract can receive Ether via payable functions, selfdestruct(), coinbase transaction or pre-sent before creation. Contract logic depending on this.balance can therefore be manipulated.
Solution: gridlock
27-fallback vs receive(): Check that all precautions and subtleties of fallback/receive functions related to visibility, state mutability and Ether transfers have been considered.
Solution:
The fallback function always receives data, but in order to also receive Ether it must be marked payable. This function must have external visibility. A fallback function can be virtual, can override and can have modifiers.
fallback () external payable
receive() external payable { ... } . This function cannot have arguments, cannot return anything and must have external visibility and payable state mutability. It can be virtual, can override and can have modifiers.
receive () external payable
28-Dangerous strict equalities: Use of strict equalities with tokens/Ether can accidentally/maliciously cause unexpected behavior.
Solution:Consider using >= or <= instead of == for such variables depending on the contract logic.
29-Locked Ether: Contracts that accept Ether via payable functions but without withdrawal mechanisms will lock up that Ether.
Solution:Remove payable attribute or add withdraw function.
30-Dangerous usage of tx.origin: Use of tx.origin for authorization may be abused by a MITM malicious contract forwarding calls from the legitimate user who interacts with it.
Solution:tx.origin should not be used for authorization. Use msg.sender instead.
31-Contract check: Checking if a call was made from an Externally Owned Account (EOA) or a contract account is typically done using extcodesize check which may be circumvented by a contract during construction when it does not have source code available.
Solution: Checking if tx.origin == msg.sender
tx.origin is the address of the EOA from which originates the transaction. While msg.sender is the address of the account from which the current call is coming from which could be an EOA or a Contract Account (CA). Ensuring that the current caller is an EOA, not a CA.
32-Deleting a mapping within a struct: Deleting a struct that contains a mapping will not delete the mapping contents which may lead to unintended consequences.
Solution: Use a lock mechanism instead of a deletion to disable structure containing a mapping.
33-Tautology or contradiction: Tautologies (always true) or contradictions (always false) indicate potential flawed logic or redundant checks
Solution:Fix the incorrect comparison by changing the value type or the comparison.
