From a2b345d9a43ff87a030227852789606e455d8bb2 Mon Sep 17 00:00:00 2001 From: devtooligan Date: Tue, 4 Jun 2024 21:26:11 -0700 Subject: [PATCH] devtooligan speedrun --- README.md | 70 +++--- vulnerabilities/authorization-txorigin.md | 2 +- vulnerabilities/default-visibility.md | 2 +- vulnerabilities/incorrect-constructor.md | 2 +- .../incorrect-inheritance-order.md | 4 +- vulnerabilities/insufficient-gas-griefing.md | 2 +- vulnerabilities/shadowing-state-variables.md | 2 +- .../unexpected-ecrecover-null-address.md | 31 ++- .../uninitialized-storage-pointer.md | 2 +- .../use-of-deprecated-functions.md | 2 +- vulnerable.sol | 217 ++++++++++++++++++ 11 files changed, 283 insertions(+), 53 deletions(-) create mode 100644 vulnerable.sol diff --git a/README.md b/README.md index 20204af..9753723 100644 --- a/README.md +++ b/README.md @@ -1,37 +1,41 @@ # Smart Contract Vulnerabilities +[DEPRECATED] +1. [Insufficient Gas Griefing](./vulnerabilities/insufficient-gas-griefing.md) +2. [Reentrancy](./vulnerabilities/reentrancy.md) +3. [Integer Overflow and Underflow](./vulnerabilities/overflow-underflow.md) +4. [Timestamp Dependence](./vulnerabilities/timestamp-dependence.md) +5. [Authorization Through tx.origin](./vulnerabilities/authorization-txorigin.md) +6. [Floating Pragma](./vulnerabilities/floating-pragma.md) +7. [Outdated Compiler Version](./vulnerabilities/outdated-compiler-version.md) +8. [Unsafe Low-Level Call](./vulnerabilities/unsafe-low-level-call.md) +9. [Unchecked Return Value](./vulnerabilities/unchecked-return-values.md) +10. [Unsupported Opcodes](./vulnerabilities/unsupported-opcodes.md) +11. [Assert Violation](./vulnerabilities/assert-violation.md) +12. [Delegatecall to Untrusted Callee](./vulnerabilities/delegatecall-untrusted-callee.md) +13. [Weak Sources of Randomness from Chain Attributes](./vulnerabilities/weak-sources-randomness.md) +14. [Signature Malleability](./vulnerabilities/signature-malleability.md) +15. [Missing Protection against Signature Replay Attacks](./vulnerabilities/missing-protection-signature-replay.md) +16. [Requirement Validation](./vulnerabilities/requirement-violation.md) +17. [Write to Arbitrary Storage Location](./vulnerabilities/arbitrary-storage-location.md) +18. [Unencrypted Private Data On-Chain](./vulnerabilities/unencrypted-private-data-on-chain.md) +19. [Inadherence to Standards](./vulnerabilities/inadherence-to-standards.md) +20. [Asserting Contract from Code Size](./vulnerabilities/asserting-contract-from-code-size.md) +21. [Transaction-Ordering Dependence](./vulnerabilities/transaction-ordering-dependence.md) +22. [DoS with Block Gas Limit](./vulnerabilities/dos-gas-limit.md) +23. [DoS with (Unexpected) revert](./vulnerabilities/dos-revert.md) +24. [Unexpected `ecrecover` null address](./vulnerabilities/unexpected-ecrecover-null-address.md) +25. [Insufficient Access Control](./vulnerabilities/insufficient-access-control.md) +26. [Off-By-One](./vulnerabilities/off-by-one.md) +27. [Lack of Precision](./vulnerabilities/lack-of-precision.md) +28. [Unbounded Return Data](./vulnerabilities/unbounded-return-data.md) -- [Insufficient Gas Griefing](./vulnerabilities/insufficient-gas-griefing.md) -- [Reentrancy](./vulnerabilities/reentrancy.md) -- [Integer Overflow and Underflow](./vulnerabilities/overflow-underflow.md) -- [Timestamp Dependence](./vulnerabilities/timestamp-dependence.md) -- [Authorization Through tx.origin](./vulnerabilities/authorization-txorigin.md) -- [Floating Pragma](./vulnerabilities/floating-pragma.md) -- [Outdated Compiler Version](./vulnerabilities/outdated-compiler-version.md) -- [Unsafe Low-Level Call](./vulnerabilities/unsafe-low-level-call.md) -- [Unchecked Return Value](./vulnerabilities/unchecked-return-values.md) -- [Unsupported Opcodes](./vulnerabilities/unsupported-opcodes.md) -- [Uninitialized Storage Pointer](./vulnerabilities/uninitialized-storage-pointer.md) -- [Assert Violation](./vulnerabilities/assert-violation.md) -- [Use of Deprecated Functions](./vulnerabilities/use-of-deprecated-functions.md) -- [Delegatecall to Untrusted Callee](./vulnerabilities/delegatecall-untrusted-callee.md) -- [Signature Malleability](./vulnerabilities/signature-malleability.md) +# Best Practices +1. [Presence of Unused Variables](./vulnerabilities/unused-variables.md) + +# Deprecated Vulerabilities +- [Default Visibility](./vulnerabilities/default-visibility.md) - [Incorrect Constructor Name](./vulnerabilities/incorrect-constructor.md) -- [Shadowing State Variables](./vulnerabilities/shadowing-state-variables.md) -- [Weak Sources of Randomness from Chain Attributes](./vulnerabilities/weak-sources-randomness.md) -- [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) - [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) -- [Inadherence to Standards](./vulnerabilities/inadherence-to-standards.md) -- [Asserting Contract from Code Size](./vulnerabilities/asserting-contract-from-code-size.md) -- [Transaction-Ordering Dependence](./vulnerabilities/transaction-ordering-dependence.md) -- [DoS with Block Gas Limit](./vulnerabilities/dos-gas-limit.md) -- [DoS with (Unexpected) revert](./vulnerabilities/dos-revert.md) -- [Unexpected `ecrecover` null address](./vulnerabilities/unexpected-ecrecover-null-address.md) -- [Default Visibility](./vulnerabilities/default-visibility.md) -- [Insufficient Access Control](./vulnerabilities/insufficient-access-control.md) -- [Off-By-One](./vulnerabilities/off-by-one.md) -- [Lack of Precision](./vulnerabilities/lack-of-precision.md) -- [Unbounded Return Data](./vulnerabilities/unbounded-return-data.md) +- [Shadowing State Variables](./vulnerabilities/shadowing-state-variables.md) +- [Uninitialized Storage Pointer](./vulnerabilities/uninitialized-storage-pointer.md) +- [Use of Deprecated Functions](./vulnerabilities/use-of-deprecated-functions.md) diff --git a/vulnerabilities/authorization-txorigin.md b/vulnerabilities/authorization-txorigin.md index ed9bcd3..cc859a9 100644 --- a/vulnerabilities/authorization-txorigin.md +++ b/vulnerabilities/authorization-txorigin.md @@ -1,6 +1,6 @@ ## Authorization Through tx.origin -`tx.origin` is a global variable in Solidity which returns the address that sent a transaction. It's important that you never use `tx.origin` for authorization since another contract can use a fallback function to call your contract and gain authorization since the authorized address is stored in `tx.origin`. Consider this example: +`tx.origin` is a global variable in Solidity which returns the address that sent a transaction. It's important that you are aware of the risk of using `tx.origin` for authorization since another contract can use a fallback function to call your contract and gain authorization since the authorized address is stored in `tx.origin`. Consider this example: ``` pragma solidity >=0.5.0 <0.7.0; diff --git a/vulnerabilities/default-visibility.md b/vulnerabilities/default-visibility.md index 16224f6..0fa7247 100644 --- a/vulnerabilities/default-visibility.md +++ b/vulnerabilities/default-visibility.md @@ -1,4 +1,4 @@ -## Default Visibility +## Default Visibility [DEPRECATED] Visibility specifiers are used to determine where a function or variable can be accessed from. As explained in the [solidity docs](https://docs.soliditylang.org/en/v0.8.15/cheatsheet.html?highlight=visibility#function-visibility-specifiers): diff --git a/vulnerabilities/incorrect-constructor.md b/vulnerabilities/incorrect-constructor.md index dbdf7d1..d9f31dc 100644 --- a/vulnerabilities/incorrect-constructor.md +++ b/vulnerabilities/incorrect-constructor.md @@ -1,4 +1,4 @@ -## Incorrect Constructor Name +## Incorrect Constructor Name [DEPRECATED] > [!NOTE] > This vulnerability is relevant to older contracts using Solidity versions before `0.4.22`. Modern Solidity versions (0.4.22 and later) use the `constructor` keyword, effectively deprecating this vulnerability. However, it is still important to be aware of this issue when reviewing or interacting with legacy contracts. diff --git a/vulnerabilities/incorrect-inheritance-order.md b/vulnerabilities/incorrect-inheritance-order.md index 95f2827..2b5f27d 100644 --- a/vulnerabilities/incorrect-inheritance-order.md +++ b/vulnerabilities/incorrect-inheritance-order.md @@ -1,6 +1,4 @@ -## Incorrect Inheritance Order - -In Solidity, it is possible to inherit from multiple sources, which if not properly understood can introduce ambiguity. This ambiguity is known as the Diamond Problem, wherein if two base contracts have the same function, which one should be prioritized? Luckily, Solidity handles this problem gracefully, that is as long as the developer understands the solution. +## Incorrect Inheritance Order [DEPRECATED] The solution Solidity provides to the Diamond Problem is by using reverse C3 linearization. This means that it will linearize the inheritance from right to left, so the order of inheritance matters. It is suggested to start with more general contracts and end with more specific contracts to avoid problems. diff --git a/vulnerabilities/insufficient-gas-griefing.md b/vulnerabilities/insufficient-gas-griefing.md index 856f757..4202e70 100644 --- a/vulnerabilities/insufficient-gas-griefing.md +++ b/vulnerabilities/insufficient-gas-griefing.md @@ -10,7 +10,7 @@ contract Relayer { function relay(bytes _data) public { // replay protection; do not call the same transaction twice - require(executed[_data] == 0, "Duplicate call"); + require(!executed[_data], "Duplicate call"); executed[_data] = true; innerContract.call(bytes4(keccak256("execute(bytes)")), _data); } diff --git a/vulnerabilities/shadowing-state-variables.md b/vulnerabilities/shadowing-state-variables.md index f5da1a0..59d774b 100644 --- a/vulnerabilities/shadowing-state-variables.md +++ b/vulnerabilities/shadowing-state-variables.md @@ -1,4 +1,4 @@ -## Shadowing State Variables +## Shadowing State Variables [DEPRECATED] It is possible to use the same variable twice in Solidity, but it can lead to unintended side effects. This is especially difficult regarding working with multiple contracts. Take the following example: diff --git a/vulnerabilities/unexpected-ecrecover-null-address.md b/vulnerabilities/unexpected-ecrecover-null-address.md index a3d2439..d6895fb 100644 --- a/vulnerabilities/unexpected-ecrecover-null-address.md +++ b/vulnerabilities/unexpected-ecrecover-null-address.md @@ -8,11 +8,17 @@ As noted above, `ecrecover` will return zero on error. It's possible to do this ``` // UNSECURE -function setOwner(bytes32 newOwner, uint8 v, bytes32 r, bytes32 s) external { - address signer = ecrecover(newOwner, v, r, s); - require(signer == owner); - owner = address(newOwner); -} + function setOwner( + address newOwner, + uint8 v, + bytes32 r, + bytes32 s + ) external { + address signer = ecrecover(keccak256(abi.encode(newOwner)), v, r, s); + require(signer == owner); + owner = address(newOwner); + } + ``` The above method is intended to only set a new `owner` if a valid signature from the existing `owner` is provided. However, as we know, if we set `v` to any value other than 27 or 28, the `signer` will be the null address and if the current owner is uninitialized or renounced, the `require` statement will succeed allowing an attacker to set themselves as `owner`. @@ -20,11 +26,16 @@ The above method is intended to only set a new `owner` if a valid signature from We can mitigate this issue by reverting if the recovered `signer` address is null, e.g.: ``` -function setOwner(bytes32 newOwner, uint8 v, bytes32 r, bytes32 s) external { - address signer = ecrecover(newOwnerHash, v, r, s); - require(signer == owner && signer != address(0)); - owner = address(newOwner); -} + function setOwner( + address newOwner, + uint8 v, + bytes32 r, + bytes32 s + ) external { + address signer = ecrecover(keccak256(abi.encode(newOwner)), v, r, s); + require(signer == owner && signer != address(0)); + owner = address(newOwner); + } ``` ### Sources diff --git a/vulnerabilities/uninitialized-storage-pointer.md b/vulnerabilities/uninitialized-storage-pointer.md index 8d87f41..280e0cb 100644 --- a/vulnerabilities/uninitialized-storage-pointer.md +++ b/vulnerabilities/uninitialized-storage-pointer.md @@ -1,4 +1,4 @@ -## Uninitialized Storage Pointer +## Uninitialized Storage Pointer [DEPRECATED] > [!NOTE] > As of solidity `0.5.0`, uninitialized storage pointers are no longer an issue since contracts with uninitialized storage pointers will no longer compile. This being said, it's still important to understand what storage pointers you should be using in certain situations. diff --git a/vulnerabilities/use-of-deprecated-functions.md b/vulnerabilities/use-of-deprecated-functions.md index 7c81533..1da9151 100644 --- a/vulnerabilities/use-of-deprecated-functions.md +++ b/vulnerabilities/use-of-deprecated-functions.md @@ -1,4 +1,4 @@ -## Use of Deprecated Functions +## Use of Deprecated Functions [DEPRECATED] As time goes by, functions and operators in Solidity are deprecated and often replaced. It's important to not use deprecated functions, as it can lead to unexpected effects and compilation errors. diff --git a/vulnerable.sol b/vulnerable.sol new file mode 100644 index 0000000..72e35b6 --- /dev/null +++ b/vulnerable.sol @@ -0,0 +1,217 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; // 6. Floating pragma // 7. Outdated Compiler Version // 10. Unsupported Opcodes + +contract ERC20 {} + +contract Vulnerable is ERC20 { + mapping(address => mapping(bytes => bool)) public executed; + mapping(address => uint) public balances; + + // 1. Insufficient Gas Griefing + function relay(bytes memory _data, address _target) external { + require(!executed[_target][_data], "Duplicate call"); + executed[_target][_data] = true; + // 8. Unsafe Low-Level Call - Unchecked call return value + // 8. Unsafe Low-Level Call - Successful call to non-existent contract + _target.call(abi.encodeWithSignature("execute(bytes)", _data)); + } + + function deposit() external payable { + // BP-1 Presence of Unused Variables + uint depositTime = block.timestamp; + + balances[msg.sender] += msg.value; + } + + // 2. Reentrancy + function withdraw(uint _amount) external { + // 16. Requirement Violation + require(balances[msg.sender] > _amount, "Insufficient balance"); + (bool success, ) = msg.sender.call{value: _amount}(""); + + // 3. Integer Overflow + unchecked { + balances[msg.sender] -= _amount; + } + + require(success, "Transfer failed"); + } + + // 4. Timestamp Dependence + function withdrawAllTimestamp(uint256 _targetTime) external { + // 11. Assert Violation + assert(block.timestamp == _targetTime); + balances[msg.sender] = 0; + (bool success, ) = msg.sender.call{value: balances[msg.sender]}(""); + require(success, "Transfer failed"); + } + + // 5. Authorization Through tx.origin + function withdrawAllTxOrigin() external { + balances[tx.origin] = 0; + // 9. Unchecked Return Value + msg.sender.call{value: balances[tx.origin]}(""); + } + + // 12. Delegatecall to Untrusted Callee + function delegateCall(address _callee, bytes memory _data) external { + (bool success, ) = _callee.delegatecall(_data); + require(success, "Delegatecall failed"); + } + + // 13. Weak Sources of Randomness from Chain Attributes + function jackpot() external returns (bool winner) { + uint seed = block.timestamp; + uint random = uint(keccak256(abi.encodePacked(seed))); + if (random % 100 == 42) { + (bool success, ) = msg.sender.call{value: address(this).balance}( + "" + ); + require(success, "Transfer failed"); + return true; + } + return false; + } + + // 14. Signature Malleability + // 15. Missing Protection against Signature Replay Attacks + mapping(bytes32 => bool) usedSignatures; + + function validateSignature( + bytes32 hash, + uint8 v, + bytes32 r, + bytes32 s + ) external returns (address) { + bytes32 key = keccak256(abi.encodePacked(v, r, s)); + usedSignatures[key] = true; + address signer = ecrecover(hash, v, r, s); + require(signer != address(0), "Invalid signature"); + return signer; + } + + // 17. Write to Arbitrary Storage Location + function writeStorage(uint256 _location, uint256 _value) external { + assembly { + sstore(_location, _value) + } + } + + // 18. Unencrypted Private Data On-Chain + string private shhhDontTell; + + function setSecret(string memory superSecretPassword) external { + shhhDontTell = superSecretPassword; + } + + function passwordProtectedWithdrawAll(string calldata password) external { + require( + keccak256(abi.encodePacked(password)) == + keccak256(abi.encodePacked(shhhDontTell)), + "Incorrect password" + ); + balances[msg.sender] = 0; + (bool success, ) = msg.sender.call{value: address(this).balance}(""); + require(success, "Transfer failed"); + } + + // 19. Inadherence to Standards + mapping(address => uint) public balanceOf; + + function transfer(address _to, uint _value) external returns (bool) { + if (balanceOf[msg.sender] < _value) { + return true; // always return true to prove we tried + } + balanceOf[msg.sender] -= _value; + balanceOf[_to] += _value; + return true; + } + + // 20. Asserting contract from Code Size + function depositEOAOnly() external payable { + require(msg.sender.code.length == 0, "Caller not EOA!"); + uint eoaBonus = msg.value / 100; + balances[msg.sender] += msg.value * eoaBonus; + } + + // 21. Transaction-Ordering Dependence + address public currentRecipient; + + function setCurrentRecipient(address _recipient) external { + currentRecipient = _recipient; + } + + function withdrawToCurrentRecipient() external { + balances[msg.sender] = 0; + (bool success, ) = currentRecipient.call{value: balances[msg.sender]}(""); + require(success, "Transfer failed"); + } + + // 22. DoS with Block Gas Limit Unbounded Operations + uint256[][] public paymentBatches; + + function submitPaymentBatch(uint256[] memory payments) external { + paymentBatches.push(payments); + } + + function processAllPaymentBatches() external { + // 26. Off-By-One Array lengths + for (uint i = 0; i < paymentBatches.length - 1; i++) { + uint256[] storage payments = paymentBatches[i]; + // 26. Off-By-One Incorrect comparison operator + for (uint j = 0; j <= payments.length; j++) { + (bool success, ) = msg.sender.call{value: payments[j]}(""); + require(success, "Transfer failed"); + } + } + } + + // 23. DoS with (Unexpected) revert - Reverting funds transfer + address[] public beneficiaries; + + function addBeneficiary(address _beneficiary) external { + beneficiaries.push(_beneficiary); + } + + function payBeneficiaries() external { + for (uint i = 0; i < beneficiaries.length; i++) { + (bool success, ) = beneficiaries[i].call{value: 1 ether}(""); + if (!success) { + revert("Transfer failed"); + } + } + } + + // 24. Unexpected `ecrecover` Null Address + // 25. Insufficient Access Control + address public owner; + + function setOwner( + address newOwner, + uint8 v, + bytes32 r, + bytes32 s + ) external { + address signer = ecrecover(keccak256(abi.encode(newOwner)), v, r, s); + require(signer == owner); + owner = address(newOwner); + } + + // 27. Lack of Precision + uint public weeklyCost = 5; + + function rentalCost( + uint numberDays + ) external payable returns (uint totalCost) { + totalCost = (weeklyCost * numberDays) / 7; + require(msg.value >= totalCost, "Insufficient funds"); + } + + // 28. Unbound Return Data + function returnBombLove(address attacker) external returns (bool) { + (bool success, ) = attacker.call{gas: 2500}( + abi.encodeWithSignature("returnExcessData()") + ); + return success; + } +}