From f4e64678717a568d06ac0f3865231cb28364eecc Mon Sep 17 00:00:00 2001 From: Lucas Janon Date: Sun, 1 May 2022 21:47:53 -0300 Subject: [PATCH 1/3] WIP: security --- README.md | 41 +++++++++++++++-------------------------- 1 file changed, 15 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 692ff6a..38dbd1c 100644 --- a/README.md +++ b/README.md @@ -1,41 +1,30 @@ # DappCamp Warriors -## Gas optimization +## Security -### Gas fees +### Overview -Turing-complete state machines are prone to the [halting problem](https://en.wikipedia.org/wiki/Halting_problem), which implies that the execution can halt at any given point in time (e.g.: a program running a `while (true)` statement). +This is the most important topic of the course, and the #1 thing to have in mind while you write smart contracts. -How does Ethereum, a decentralized, permissionless, Turing-complete machine prevent people from halting it? -By imposing a gas fee on every computation. +At this point, I think the why is clear, so let's go to the how. -Gas fees are essential to the Ethereum network. They: +### Writing secure code -* Prevent bad actors from spamming the network. -* Can be adjusted to tip the miners and regulate transaction priority. -* Are partially burned to diminish the Ether supply. +#### Rules of thumb -### Why we optimize gas fees +##### Test your code -When computation and storage have a relevant monetary cost, optimizing them becomes important. We may argue that with future upgrades, using the Ethereum network will get cheaper, but that's not the case for now, so gas optimization is key. +Just go and test it, preferably before writing it. -I like to divide gas optimizations in two groups: +##### Use audited code -* Contract deploy gas optimizations. -* Runtime gas optimizations. +* Use [OpenZeppelin's contracts](https://github.com/OpenZeppelin/openzeppelin-contracts). +* Try to find an audited version of what you're trying to do in [Certik's website](https://www.certik.com/). -If thousands of people use your contracts everyday, runtime gas optimizations may save your users millions of dollars. On the other side, deploy gas optimizations are used just once1, but some contracts, depending on length and initial parameters, can cost thousands of dollars to deploy. +##### Favor security over performance -1. Note that contracts [can be deployed programmatically by other contracts](https://github.com/Uniswap/v3-core/blob/ed88be38ab2032d82bf10ac6f8d03aa631889d48/contracts/UniswapV3PoolDeployer.sol#L35), in that case, optimizing deploy gas usage also optimizes runtime consumption. +Like we mentioned previously, gas consumption is a good variable to optimize for. But, if optimizing cost requires writing indirect, hard to follow code, take care and make sure you know what you're doing. Note that this heuristic doesn't imply that you should add a [`nonReentrant` modifier](https://docs.openzeppelin.com/contracts/2.x/api/utils#ReentrancyGuard-nonReentrant--) on every function of your contract. -### Optimizing gas fees +##### Make your code obvious -In the same way you can save gasoil if you accelerate your car gently, you can save gas by using the techniques we cover on this branch. - -#### Preferred variable size - -When in doubt, use 256 bit variables like `uint256` or `bytes32`. - -This may sound counterintuitive since at first glance, `uint256` takes more resources than `uint16`. But most of the times it doesn't, since EVM's storage slots have 256 bits, so smaller variables are padded with zeros by the EVM and that has a gas cost. The only case in which you want to use smaller variables is when you are [packing variables](#packing-variables). - -##### Packing variables +Maybe you learned a [bit manipulation](https://en.wikipedia.org/wiki/Bit_manipulation) trick a few days ago and gas optimization seems like a good excuse to try it. Don't. Your code may be readable for you, but it will definitely cause unneeded cognitive overload to your auditors or code reviewers, or even to you in a few weeks when you're deploying your code. From 02ccb0f053dc97a83f22b4a94fb5f4317db31a9f Mon Sep 17 00:00:00 2001 From: Lucas Janon Date: Sun, 8 May 2022 20:57:54 -0300 Subject: [PATCH 2/3] Add security contracts and docs --- README.md | 38 +++++++++++++++++++++--------- contracts/security/Insecure.sol | 13 +++++++++++ contracts/security/Secure.sol | 41 +++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 11 deletions(-) create mode 100644 contracts/security/Insecure.sol create mode 100644 contracts/security/Secure.sol diff --git a/README.md b/README.md index 38dbd1c..0bb3b27 100644 --- a/README.md +++ b/README.md @@ -2,29 +2,45 @@ ## Security -### Overview - This is the most important topic of the course, and the #1 thing to have in mind while you write smart contracts. At this point, I think the why is clear, so let's go to the how. -### Writing secure code - -#### Rules of thumb +### Rules of thumb for writing secure code -##### Test your code +#### Test your code -Just go and test it, preferably before writing it. +Aim for +90% coverage. Preferably, test it before writing it. -##### Use audited code +#### Use audited code * Use [OpenZeppelin's contracts](https://github.com/OpenZeppelin/openzeppelin-contracts). * Try to find an audited version of what you're trying to do in [Certik's website](https://www.certik.com/). -##### Favor security over performance +#### Favor security over performance + +Like we mentioned previously, gas consumption is a good variable to optimize for. But, if optimizing cost requires writing unreadable code or skipping security measures, take care and make sure you know what you're doing. -Like we mentioned previously, gas consumption is a good variable to optimize for. But, if optimizing cost requires writing indirect, hard to follow code, take care and make sure you know what you're doing. Note that this heuristic doesn't imply that you should add a [`nonReentrant` modifier](https://docs.openzeppelin.com/contracts/2.x/api/utils#ReentrancyGuard-nonReentrant--) on every function of your contract. +Note that this heuristic doesn't imply that your code should be full of naive and costly security measures (like adding a [`nonReentrant` modifier](https://docs.openzeppelin.com/contracts/2.x/api/utils#ReentrancyGuard-nonReentrant--) on every function). -##### Make your code obvious +#### Make your code obvious Maybe you learned a [bit manipulation](https://en.wikipedia.org/wiki/Bit_manipulation) trick a few days ago and gas optimization seems like a good excuse to try it. Don't. Your code may be readable for you, but it will definitely cause unneeded cognitive overload to your auditors or code reviewers, or even to you in a few weeks when you're deploying your code. + + + +### Security resources + +* [Consensys security best practices](https://consensys.github.io/smart-contract-best-practices/) + +* [Smart Contract Weakness Classification and Test Cases](https://swcregistry.io/) + +* [Security considerations (Solidity docs)](https://docs.soliditylang.org/en/develop/security-considerations.html) + +* [Twitter thread about security](https://twitter.com/The3D_/status/1485308693935763458) + +* [OpenZeppelin audits](https://blog.openzeppelin.com/security-audits/) + +* [Certik audits](https://www.certik.com/) + +* [How to become a smart contract auditor](https://cmichel.io/how-to-become-a-smart-contract-auditor/) diff --git a/contracts/security/Insecure.sol b/contracts/security/Insecure.sol new file mode 100644 index 0000000..3baee7c --- /dev/null +++ b/contracts/security/Insecure.sol @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.4; + +contract Insecure { + mapping(address => uint256) private userBalances; + + function withdrawBalance() public { + uint256 amountToWithdraw = userBalances[msg.sender]; + (bool success, ) = msg.sender.call{value: amountToWithdraw}(""); + require(success, "Error withdrawing balance."); + userBalances[msg.sender] = 0; + } +} diff --git a/contracts/security/Secure.sol b/contracts/security/Secure.sol new file mode 100644 index 0000000..a11263a --- /dev/null +++ b/contracts/security/Secure.sol @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: MIT +/** + * @dev Use a deterministic compiler version. + * Using a caret ^ in the contract version implies that your contract + * can be compiled with multiple compiler versions (^0.8.13 -> 0.*.*). + */ +pragma solidity 0.8.13; + +import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; + +contract Secure is ReentrancyGuard { + mapping(address => uint256) private userBalances; + + /** + * @dev The insecure `withdrawBalance` implementation was prone to a reentrancy attack. + * Below you can find two possible ways to prevent it. + * See: https://consensys.github.io/smart-contract-best-practices/attacks/reentrancy/ + */ + + function withdrawBalance1() public { + uint256 amountToWithdraw = userBalances[msg.sender]; + /** + * @dev Prevent reentrancy by setting contract state and then performing the transfer. + * See: https://docs.soliditylang.org/en/latest/security-considerations.html#use-the-checks-effects-interactions-pattern + */ + userBalances[msg.sender] = 0; + (bool success, ) = msg.sender.call{value: amountToWithdraw}(""); + require(success, "Error withdrawing balance."); + } + + /** + * @dev Prevent reentrancy by using OpenZeppelin's ReentrancyGuard. + * See: https://docs.openzeppelin.com/contracts/2.x/api/utils#ReentrancyGuard + */ + function withdrawBalance2() public nonReentrant { + uint256 amountToWithdraw = userBalances[msg.sender]; + (bool success, ) = msg.sender.call{value: amountToWithdraw}(""); + require(success, "Error withdrawing balance."); + userBalances[msg.sender] = 0; + } +} From 61bafdbbf5aa47942901ba1fc46968283c0195ae Mon Sep 17 00:00:00 2001 From: Lucas Janon Date: Thu, 12 May 2022 09:55:52 -0300 Subject: [PATCH 3/3] Update README, add more cases to contracts --- README.md | 26 +++++++++++++++++++++++++- contracts/security/Insecure.sol | 21 ++++++++++++++++++++- contracts/security/Secure.sol | 31 +++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 0bb3b27..6d2faf5 100644 --- a/README.md +++ b/README.md @@ -23,10 +23,34 @@ Like we mentioned previously, gas consumption is a good variable to optimize for Note that this heuristic doesn't imply that your code should be full of naive and costly security measures (like adding a [`nonReentrant` modifier](https://docs.openzeppelin.com/contracts/2.x/api/utils#ReentrancyGuard-nonReentrant--) on every function). -#### Make your code obvious +#### Make your code readable Maybe you learned a [bit manipulation](https://en.wikipedia.org/wiki/Bit_manipulation) trick a few days ago and gas optimization seems like a good excuse to try it. Don't. Your code may be readable for you, but it will definitely cause unneeded cognitive overload to your auditors or code reviewers, or even to you in a few weeks when you're deploying your code. +#### Make your code obvious + +Like Joel Spolsky said, [make your wrong code look wrong](https://www.joelonsoftware.com/2005/05/11/making-wrong-code-look-wrong/). +If your contract calls untrusted third-party contracts, make it explicit: + +```solidity +// bad +Bank.withdraw(100); // Unclear whether trusted or untrusted + +function makeWithdrawal(uint amount) { // Isn't clear that this function is potentially unsafe + Bank.withdraw(amount); +} + +// good +UntrustedBank.withdraw(100); // untrusted external call +TrustedBank.withdraw(100); // external but trusted bank contract maintained by XYZ Corp + +function makeUntrustedWithdrawal(uint amount) { + UntrustedBank.withdraw(amount); +} +``` + +> Code snippet from [Consensys](https://consensys.github.io/smart-contract-best-practices/development-recommendations/general/external-calls/#mark-untrusted-contracts). + ### Security resources diff --git a/contracts/security/Insecure.sol b/contracts/security/Insecure.sol index 3baee7c..0a2bc78 100644 --- a/contracts/security/Insecure.sol +++ b/contracts/security/Insecure.sol @@ -4,10 +4,29 @@ pragma solidity ^0.8.4; contract Insecure { mapping(address => uint256) private userBalances; - function withdrawBalance() public { + function unsafeWithdrawBalance() public { uint256 amountToWithdraw = userBalances[msg.sender]; (bool success, ) = msg.sender.call{value: amountToWithdraw}(""); require(success, "Error withdrawing balance."); userBalances[msg.sender] = 0; } + + address public highestBidder; + uint256 public highestBid; + + function unsafeBid() public payable { + require(msg.value >= highestBid, "Bid is too low"); + + if (highestBidder != address(0)) { + /** + * @dev We're calling a user-provided address, we don't know if it's a contract or an EOA. + * If it's a contract and this call fails consistently, no one else can bid. + */ + (bool success, ) = highestBidder.call{value: highestBid}(""); + require(success, "Error bidding"); + } + + highestBidder = msg.sender; + highestBid = msg.value; + } } diff --git a/contracts/security/Secure.sol b/contracts/security/Secure.sol index a11263a..6fdc641 100644 --- a/contracts/security/Secure.sol +++ b/contracts/security/Secure.sol @@ -38,4 +38,35 @@ contract Secure is ReentrancyGuard { require(success, "Error withdrawing balance."); userBalances[msg.sender] = 0; } + + /** + * @dev Pull don't push + * See: https://consensys.github.io/smart-contract-best-practices/development-recommendations/general/external-calls/#favor-pull-over-push-for-external-calls + */ + + address public highestBidder; + uint256 public highestBid; + mapping(address => uint256) public refunds; + + function bid() public payable { + require(msg.value >= highestBid, "Bid is too low"); + + if (highestBidder != address(0)) { + refunds[highestBidder] += highestBid; + } + + highestBidder = msg.sender; + highestBid = msg.value; + } + + /** + * @dev Make the users of your contract pull their balance. + * Don't push it to them since it allows them to manipulate your bid. + */ + function withdrawRefund() external { + uint256 refund = refunds[msg.sender]; + refunds[msg.sender] = 0; + (bool success, ) = msg.sender.call{value: refund}(""); + require(success, "Can't withdraw"); + } }