Merge branch 'master' into msgValue

README.md

@@ -21,6 +21,7 @@
 - [Missing Protection against Signature Replay Attacks](./vulnerabilities/missing-protection-signature-replay.md)
 - [Requirement Validation](./vulnerabilities/requirement-violation.md)
 - [Write to Arbitrary Storage Location](./vulnerabilities/arbitrary-storage-location.md)
+- [Hash Collision when using abi.encodePacked() with Multiple Variable-Length Arguments](./vulnerabilities/hash-collision.md)
 - [Incorrect Inheritance Order](./vulnerabilities/incorrect-inheritance-order.md)
 - [Presence of Unused Variables](./vulnerabilities/unused-variables.md)
 - [Unencrypted Private Data On-Chain](./vulnerabilities/unencrypted-private-data-on-chain.md)
@@ -36,3 +37,4 @@
 - [Lack of Precision](./vulnerabilities/lack-of-precision.md)
 - [Unbounded Return Data](./vulnerabilities/unbounded-return-data.md)
 - [Using ``msg.value`` in a Loop](./vulnerabilities/msgvalue-loop.md)
+- [Deleting a Mapping Within a Struct](./vulnerabilities/mapping-within-struct.md)

vulnerabilities/hash-collision.md

@@ -0,0 +1,146 @@
+ # Hash Collision when using `abi.encodePacked()` with Multiple Variable-Length Arguments
+
+In Solidity, the `abi.encodePacked()` function is used to create tightly packed byte arrays which can then be hashed using `keccak256()`
+
+However, this function can be dangerous when used with multiple variable-length arguments because it can lead to hash collisions. These collisions can potentially be exploited in scenarios such as signature verification, allowing attackers to bypass authorization mechanisms.
+
+**Hash Collision** is a situation where two different sets of inputs produce the same hash output. In this context, a hash collision can occur when using `abi.encodePacked()` with multiple variable-length arguments, allowing an attacker to craft different inputs that produce the same hash.
+
+## Understanding the vulnerability
+
+When `abi.encodePacked()` is used with multiple variable-length arguments (such as arrays and strings), the packed encoding does not include information about the boundaries between different arguments. This can lead to situations where different combinations of arguments result in the same encoded output, causing hash collisions.
+
+For example, consider the following two calls to `abi.encodePacked()`:
+
+```
+abi.encodePacked(["a", "b"], ["c", "d"])
+```
+
+```
+abi.encodePacked(["a"], ["b", "c", "d"])
+```
+
+Both calls could potentially produce the same packed encoding because `abi.encodePacked()` simply concatenates the elements without any delimiters!
+
+Consider the below example for strings:
+
+```
+abi.encodePacked("foo", "bar") == abi.encodePacked("fo", "obar")
+```
+
+Strings in Solidity are dynamic types and when they are concatenated using `abi.encodePacked()`, there is no delimiter between them to mark their boundaries, which can lead to hash collisions.
+
+As a matter of fact, the below warning is taken as it is straight from the [official solidity language documentation](https://docs.soliditylang.org/en/latest/abi-spec.html#non-standard-packed-mode) regarding the same.
+
+
+> [!WARNING]
+> If you use `keccak256(abi.encodePacked(a, b))` and both `a` and `b` are dynamic types, it is easy to craft collisions in the hash value by moving parts of `a` into `b` and vice-versa.
+> More specifically, `abi.encodePacked("a", "bc") == abi.encodePacked("ab", "c")`. If you use `abi.encodePacked` for signatures, authentication or data integrity, make sure to always use the same types and check that at most one of them is dynamic. Unless there is a compelling reason, `abi.encode` should be preferred.
+
+
+## Sample Code Analysis
+
+
+```solidity
+/// INSECURE
+function addUsers(address[] calldata admins, address[] calldata regularUsers, bytes calldata signature) external {
+    if (!isAdmin[msg.sender]) {
+        bytes32 hash = keccak256(abi.encodePacked(admins, regularUsers));
+        address signer = hash.toEthSignedMessageHash().recover(signature);
+        require(isAdmin[signer], "Only admins can add users.");
+    }
+    for (uint256 i = 0; i < admins.length; i++) {
+        isAdmin[admins[i]] = true;
+    }
+    for (uint256 i = 0; i < regularUsers.length; i++) {
+        isRegularUser[regularUsers[i]] = true;
+    }
+}
+```
+
+In the provided sample code above, the `addUsers` function uses `abi.encodePacked(admins, regularUsers)` to generate a hash. An attacker could exploit this by rearranging elements between the `admins` and `regularUsers` arrays, resulting in the same hash and thereby bypassing authorization checks.
+
+```solidity
+/// INSECURE
+function verifyMessage(string calldata message1, string calldata message2, bytes calldata signature) external {
+    bytes32 hash = keccak256(abi.encodePacked(message1, message2));
+    address signer = hash.toEthSignedMessageHash().recover(signature);
+    require(isAuthorized[signer], "Unauthorized signer");
+}
+```
+
+The above function `verifyMessage()` could easily be exploited as below:-
+
+```
+verifyMessage("hello", "world", signature);
+```
+or
+
+```
+verifyMessage("hell", "oworld", signature);
+```
+
+or a variation of the string `hello` `world`
+
+All variations of the string `hello` `world` passed to `verifyMessage()` would produce the same hash, potentially allowing an attacker to bypass the authorization check if they can provide a valid signature for their manipulated inputs.
+
+**Fixed Code Using Single User:**
+
+```solidity
+function addUser(address user, bool admin, bytes calldata signature) external {
+    if (!isAdmin[msg.sender]) {
+        bytes32 hash = keccak256(abi.encodePacked(user));
+        address signer = hash.toEthSignedMessageHash().recover(signature);
+        require(isAdmin[signer], "Only admins can add users.");
+    }
+    if (admin) {
+        isAdmin[user] = true;
+    } else {
+        isRegularUser[user] = true;
+    }
+}
+```
+
+This approach eliminates the use of variable-length arrays, thus avoiding the hash collision issue entirely by dealing with a single user at a time.
+
+**Fixed Code Using Fixed-Length Arrays:**
+
+```solidity
+function addUsers(address[3] calldata admins, address[3] calldata regularUsers, bytes calldata signature) external {
+    if (!isAdmin[msg.sender]) {
+        bytes32 hash = keccak256(abi.encodePacked(admins, regularUsers));
+        address signer = hash.toEthSignedMessageHash().recover(signature);
+        require(isAdmin[signer], "Only admins can add users.");
+    }
+    for (uint256 i = 0; i < admins.length; i++) {
+        isAdmin[admins[i]] = true;
+    }
+    for (uint256 i = 0; i < regularUsers.length; i++) {
+        isRegularUser[regularUsers[i]] = true;
+    }
+}
+```
+
+In this version, fixed-length arrays are used, which mitigates the risk of hash collisions since the encoding is unambiguous.
+
+
+## Remediation Strategies
+
+To prevent this type of hash collision, the below remediation strategies can be employed:
+
+1. **Avoid Variable-Length Arguments**: Avoid using `abi.encodePacked()` with variable-length arguments such as arrays and strings. Instead, use fixed-length arrays to ensure the encoding is unique and unambiguous.
+
+2. **Use `abi.encode()` Instead**: Unlike `abi.encodePacked()`, `abi.encode()` includes additional type information and length prefixes in the encoding, making it much less prone to hash collisions. Switching from `abi.encodePacked()` to `abi.encode()` is a simple yet effective fix.
+
+> [!IMPORTANT]
+> Replay Protection does not protect against possible hash collisions!
+> 
+> It is listed here as a defense in depth strategy and SHOULD NOT be solely relied upon to protect against said vulnerability
+
+3. **Replay Protection**: Implement replay protection mechanisms to prevent attackers from reusing valid signatures. This can involve including nonces or timestamps in the signed data. However, this does not completely eliminate the risk of hash collisions but adds an additional layer of security. More on this can be found [here](./missing-protection-signature-replay.md)
+
+
+
+## Sources
+- [Smart Contract Weakness Classification #133](https://swcregistry.io/docs/SWC-133/)
+- [Solidity Non-standard Packed Mode](https://docs.soliditylang.org/en/latest/abi-spec.html#non-standard-packed-mode)

vulnerabilities/mapping-within-struct.md

@@ -0,0 +1,24 @@
+# Deleting a Mapping Within a Struct
+
+It is a common assumption that deleting a ``struct`` will delete all of it's data entirely but there is an exception. Deleting structs with dynamic data types does not delete the data stored inside them.
+
+For example: If a ``mapping`` (or dynamic array) is inside a struct, and the struct is deleted, the mapping will not be deleted. This is because mappings are implemented as hash tables and the EVM does not keep track of which keys have been used in the mapping. As a result, EVM doesn't know how to reset a mapping and the remaining data can be used to compromise the contract. 
+
+```solidity
+    struct BalancesStruct{
+        address owner;
+        mapping(address => uint) balances;
+    }
+
+    mapping(address => BalancesStruct) public stackBalance;
+
+    function remove() internal{
+         delete stackBalance[msg.sender]; // doesn't delete balances mapping inside BalancesStruct
+    }
+```
+``remove()`` function above deletes an item of ``stackBalance``. But the mapping ``balances`` inside ``BalancesStruct`` won't reset. Only individual keys and what they map to can be deleted. Example: ``delete stackBalance[msg.sender].balances[x]`` will delete the data stored at address ``x`` in the balances mapping.
+
+
+
+## Sources
+- https://docs.soliditylang.org/en/latest/types.html#delete

vulnerabilities/overflow-underflow.md

@@ -1,18 +1,73 @@
 ## Integer Overflow and Underflow
+In solidity, Integer types have maximum and minimum values. Integer overflow occurs when an integer variable exceeds the maximum value that can be stored in that variable type. Similarly, Integer underflow occurs when an integer variable goes below the minimum value for that variable type. Example: The maximum value ``uint8`` can store is ``255``. Now, when you store ``256`` in ``uint8`` it will overflow and the value will reset to 0. When you store ``257``, the value will be ``1``, ``2`` for ``258`` and so on. Similarly, if you try to store ``-1`` in the uint8 variable the value of the variable will become ``255``, and so on as it will underflow.
 
-In solidity, integer types have maximum values. For example:
+Some integer types and their min/max values:
+| Type   |      Max      |  Min |
+|----------|-------------|------|
+| uint8 |  255 | 0 |
+| uint16 | 65535 |   0 |
+| uint24 | 16777215 | 0 |
+| uint256 | 2^256 - 1 |  0 |
 
-`uint8` => 255
-`uint16` => 65535
-`uint24` => 16777215
-`uint256` => (2^256) - 1
+Since smaller integer types like: ``uint8``, ``uint16``, etc have smaller maximum values, it can be easier to cause an overflow, thus they should be used with greater caution.
 
-Overflow and underflow bugs can occur when you exceed the maximum value (overflow) or when you go below the minimum value (underflow). When you exceed the maximum value, you go back down to zero, and when you go below the minimum value, it brings you back up to the maximum value.
+To prevent over/underflows, [Safe Math Library](https://github.com/ConsenSysMesh/openzeppelin-solidity/blob/master/contracts/math/SafeMath.sol) is often used by contracts with older versions of Solidity but Solidity >=0.8 protects against integer overflows and underflows through the use of built-in safe math functions. It's important to consider that regardless of SafeMath logic being used, either built-in or used manually in older contracts, over/underflows still trigger reverts, which may result in [denial of service](https://github.com/kadenzipfel/smart-contract-vulnerabilities/blob/master/vulnerabilities/dos-revert.md) of important functionality or other unexpected effects. Even after the update of solidity to 0.8, there are scenarios in which the integer overflow and underflow can still occur without the transaction reverting.
 
-Since smaller integer types like: `uint8`, `uint16`, etc. have smaller maximum values, it can be easier to cause an overflow, thus they should be used with greater caution.
+### Typecasting
+The most common way in which integer over/underflow is possible when you convert an integer of a larger uint data type to a smaller data type.
+```solidity
+uint256 public a = 258;
+uint8 public b = uint8(a); // typecasting uint256 to uint8
+```
 
-Older contracts often made use of the [SafeMath library](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/math/SafeMath.sol), to avoid over/underflows, but in solidity >=v0.8.0, SafeMath logic is built in by default. It's important to consider that regardless of SafeMath logic being used, either built-in or used manually in older contracts, over/underflows still trigger reverts, which may result in [denial of service](./dos-revert.md) of important functionality or other unexpected effects. Built-in SafeMath logic may also be avoided with `unchecked` blocks, [see docs for more info](https://docs.soliditylang.org/en/v0.8.15/control-structures.html?highlight=unchecked#checked-or-unchecked-arithmetic).
+The above code snippet will overflow and the ``2`` will be stored in the variable ``b`` due to the fact that maximum value in uint8 data type is ``255``. So, it will overflow and reset to ``0`` without reverting.
 
-### Sources
+### Using Shift Operators
+Overflow & underflow checks are not performed for shift operations like they are performed for other arithmetic operations. Thus, over/underflows can occur.
 
-- https://consensys.github.io/smart-contract-best-practices/attacks/insecure-arithmetic/
+The left shift ``<<`` operator shifts all the beats in the first operand by the number specified in the second operand. Shifting an operand by 1 position is equivalent to multiplying it by 2, shifting 2 positions is equivalent to multiplying it by 4, and shifting 3 positions is equivalent to multiplying by 8. 
+
+```solidity
+uint8 public a = 100;
+uint8 public b = 2;
+
+uint8 public c = a << b; // overflow as 100 * 4 > 255
+```
+In the above code, left shifting ``a`` which is ``100`` by 2 positions ``b`` is equivalent to multiplying 100 by 4. So the result will overflow and the value in c will be ``144`` because ``400-256`` is ``144``.
+
+### Use of Inline Assembly:
+Inline Assembly in solidity is performed using YUL language. In YUL programming language, integer underflow & overflow is possible as compiler does not check automatically for it as YUL is a low-level language that is mostly used for making the code more optimized, which does this by omitting many opcodes.
+
+```solidity
+uint8 public a = 255;
+
+function addition() public returns (uint8 result) {
+    assembly {
+        result := add(sload(a.slot), 1) // adding 1 will overflow and reset to 0
+        // using inline assembly
+    }
+
+    return result;
+}
+```
+In the above code we are adding ``1`` into the variable with inline assembly and then returning the result. The variable result will overflow and 0 will be returned, despite this the contract will not throw an error or revert.
+
+### Use of unchecked code block:
+Performing arithmetic operations inside the unchecked block saves a lot of gas because it omits several checks and opcodes. But, some of these opcodes are used in default arithmetic operations in 0.8 to check for underflow/overflow.
+
+```solidity
+uint8 public a = 255;
+
+function uncheck() public{
+
+  unchecked {
+      a++;  // overflow and reset to 0 without reverting
+  }
+
+}
+```
+The unchecked code block is only recommended if you are sure that there is no possible way for the arithmetic to overflow or underflow.
+
+## Sources
+1. https://docs.soliditylang.org/en/latest/080-breaking-changes.html
+2. https://faizannehal.medium.com/how-solidity-0-8-protect-against-integer-underflow-overflow-and-how-they-can-still-happen-7be22c4ab92f

vulnerabilities/use-of-deprecated-functions.md

@@ -6,7 +6,7 @@ Here is a *non-exhaustive* list of deprecated functions and alternatives. Many a
 
 | Deprecated              | Alternatives              |
 | :---------------------- | ------------------------: |
-| `suicide(address)`      |	`selfdestruct(address)`   |
+| `suicide(address)`/`selfdestruct(address)` | N/A    | 
 | `block.blockhash(uint)` |	`blockhash(uint)`         |
 | `sha3(...)`	            | `keccak256(...)`          |
 | `callcode(...)`	        | `delegatecall(...)`       |
@@ -18,4 +18,4 @@ Here is a *non-exhaustive* list of deprecated functions and alternatives. Many a
 ### Sources
 
 - https://swcregistry.io/docs/SWC-111
-- https://github.com/ethereum/solidity/releases
+- https://github.com/ethereum/solidity/releases