From 06af1a3642e31202250ac14bbd66ef79a00cd049 Mon Sep 17 00:00:00 2001 From: 0xTaylor <> Date: Wed, 24 Nov 2021 02:40:48 -0800 Subject: [PATCH] fixes --- 7. Audit Findings 101.md | 2 +- ...us adapter can access users\342\200\231 tokens.md" | 2 +- ...ting fallback function will lock up all payouts.md | 2 +- ...nd order matching may lead to exchange griefing.md | 2 +- .../Creating proposal is not trustless.md | 2 +- .../Delegate assignment front-running.md | 2 +- ...kens with no return value will fail to transfer.md | 2 +- ...ket`, `Comptroller.exitMarket` are not checked.md" | 5 ++++- .../Flash minting can be used to redeem `fyDAI`.md | 6 +++++- .../Improper Supply Cap Limitation Enforcement.md | 2 +- .../Initialization functions can be front-run.md | 6 +++++- ...urn value checks can lead to unexpected results.md | 2 +- ...for critical operations leaves them error-prone.md | 4 +++- .../Overflow-underflow protection.md | 2 +- ...wner can front-run traders by updating adapters.md | 2 +- ...ed separately and block `Proposal.execute` call.md | 2 +- ...sing and committing still possible after launch.md | 2 +- .../7. Audit Findings 101/Random task execution.md | 2 +- .../Reentrancy vulnerability in `MetaSwap.swap()`.md | 2 +- ... order of parameters in allowance function call.md | 2 +- .../Sponsorship front-running.md | 2 +- .../Summoner can steal funds using bailout.md | 4 +++- ...ows for control of the pool\342\200\231s price.md" | 2 +- ...h more than 18 decimal points will cause issues.md | 4 +++- ...nchecked return value for `IWETH.transfer` call.md | 2 +- ... return values of `transfer` and `transferFrom`.md | 2 +- .../7. Audit Findings 101/Whitelisted tokens limit.md | 11 +++++++++-- ...urve` allows users to acquire FEI before launch.md | 1 - ...()` accepts deposits of zero, blocking the pool.md | 2 +- ....commit` overwrites previously-committed values.md | 2 +- ....emergencyExit` remains functional after launch.md | 2 +- .../`Saferagequit` makes you lose funds.md | 2 +- ...urns true if the timer has not been initialized.md | 6 ++++-- ...niswapIncentive` overflow on pre-transfer hooks.md | 6 ++++-- ...xactTokensForETH` checks the wrong return value.md | 2 +- 35 files changed, 65 insertions(+), 38 deletions(-) diff --git a/7. Audit Findings 101.md b/7. Audit Findings 101.md index f377c83c..f02f95ad 100644 --- a/7. Audit Findings 101.md +++ b/7. Audit Findings 101.md @@ -11,7 +11,7 @@ 11. [`UniswapIncentive` overflow on pre-transfer hooks](content/7.%20Audit%20Findings%20101/`UniswapIncentive`%20overflow%20on%20pre-transfer%20hooks.md) 12. [`BondingCurve` allows users to acquire FEI before launch](content/7.%20Audit%20Findings%20101/`BondingCurve`%20allows%20users%20to%20acquire%20FEI%20before%20launch.md) 13. [`Timed.isTimeEnded` returns true if the timer has not been initialized](content/7.%20Audit%20Findings%20101/`Timed.isTimeEnded`%20returns%20true%20if%20the%20timer%20has%20not%20been%20initialized.md) -14. [Overflow/underflow protection](content/7.%20Audit%20Findings%20101/Overflow/underflow%20protection.md) +14. [Overflow/underflow protection](content/7.%20Audit%20Findings%20101/Overflow-underflow%20protection.md) 15. [Unchecked return value for `IWETH.transfer` call](content/7.%20Audit%20Findings%20101/Unchecked%20return%20value%20for%20`IWETH.transfer`%20call.md) 16. [`GenesisGroup.emergencyExit` remains functional after launch](content/7.%20Audit%20Findings%20101/`GenesisGroup.emergencyExit`%20remains%20functional%20after%20launch.md) 17. [ERC20 tokens with no return value will fail to transfer](content/7.%20Audit%20Findings%20101/ERC20%20tokens%20with%20no%20return%20value%20will%20fail%20to%20transfer.md) diff --git "a/content/7. Audit Findings 101/A new malicious adapter can access users\342\200\231 tokens.md" "b/content/7. Audit Findings 101/A new malicious adapter can access users\342\200\231 tokens.md" index 7fef2a04..c10732c0 100644 --- "a/content/7. Audit Findings 101/A new malicious adapter can access users\342\200\231 tokens.md" +++ "b/content/7. Audit Findings 101/A new malicious adapter can access users\342\200\231 tokens.md" @@ -1,7 +1,7 @@ # 19 - [A new malicious adapter can access users’ tokens](./A%20new%20malicious%20adapter%20can%20access%20users’%20tokens.md) -A new malicious adapter can access users’ tokens The purpose of the `MetaSwap` contract is to save users gas costs when dealing with a number of different aggregators. +The purpose of the `MetaSwap` contract is to save users gas costs when dealing with a number of different aggregators. They can just `approve()` their tokens to be spent by `MetaSwap` (or in a later architecture, the Spender contract). They can then perform trades with all supported aggregators without having to reapprove anything. diff --git a/content/7. Audit Findings 101/A reverting fallback function will lock up all payouts.md b/content/7. Audit Findings 101/A reverting fallback function will lock up all payouts.md index 96aafcf1..f1387644 100644 --- a/content/7. Audit Findings 101/A reverting fallback function will lock up all payouts.md +++ b/content/7. Audit Findings 101/A reverting fallback function will lock up all payouts.md @@ -1,7 +1,7 @@ # 25 - [A reverting fallback function will lock up all payouts](./A%20reverting%20fallback%20function%20will%20lock%20up%20all%20payouts.md) -A reverting fallback function will lock up all payouts In `BoxExchange.sol`, the internal function `_transferEth()` reverts if the transfer does not succeed. +In `BoxExchange.sol`, the internal function `_transferEth()` reverts if the transfer does not succeed. The `_payment()` function processes a list of transfers to settle the transactions in an `ExchangeBox`. diff --git a/content/7. Audit Findings 101/Batch processing of transaction execution and order matching may lead to exchange griefing.md b/content/7. Audit Findings 101/Batch processing of transaction execution and order matching may lead to exchange griefing.md index 91f0fbc7..9afc0ba0 100644 --- a/content/7. Audit Findings 101/Batch processing of transaction execution and order matching may lead to exchange griefing.md +++ b/content/7. Audit Findings 101/Batch processing of transaction execution and order matching may lead to exchange griefing.md @@ -16,7 +16,7 @@ ___ ![064.png](../../images/7.%20Audit%20Findings%20101/064.png) ___ ## Slide Text -- ToB Audit Ox Protocol Finding 3 +- ToB Audit Ox Protocol Finding 6 - DoS - Medium Severity - Batch tx Processing diff --git a/content/7. Audit Findings 101/Creating proposal is not trustless.md b/content/7. Audit Findings 101/Creating proposal is not trustless.md index 2de7af09..25d240cc 100644 --- a/content/7. Audit Findings 101/Creating proposal is not trustless.md +++ b/content/7. Audit Findings 101/Creating proposal is not trustless.md @@ -1,7 +1,7 @@ # 27 - [Creating proposal is not trustless](./Creating%20proposal%20is%20not%20trustless.md) -Creating proposal is not trustless Usually, if someone submits a proposal and transfers some amount of tribute tokens, these tokens are transferred back if the proposal is rejected. +Usually, if someone submits a proposal and transfers some amount of tribute tokens, these tokens are transferred back if the proposal is rejected. But if the proposal is not processed before the emergency processing, these tokens will not be transferred back to the proposer. diff --git a/content/7. Audit Findings 101/Delegate assignment front-running.md b/content/7. Audit Findings 101/Delegate assignment front-running.md index 2aa1242e..24276d18 100644 --- a/content/7. Audit Findings 101/Delegate assignment front-running.md +++ b/content/7. Audit Findings 101/Delegate assignment front-running.md @@ -14,7 +14,7 @@ ___ ![033.png](../../images/7.%20Audit%20Findings%20101/033.png) ___ ## Slide Text -- ConsenSys Audit The Lao Finding 5.4 +- ConsenSys Audit The Lao Finding 5.8 - Timing & DoS - Major Severity - Front-running diff --git a/content/7. Audit Findings 101/ERC20 tokens with no return value will fail to transfer.md b/content/7. Audit Findings 101/ERC20 tokens with no return value will fail to transfer.md index 03368806..0e2f61d9 100644 --- a/content/7. Audit Findings 101/ERC20 tokens with no return value will fail to transfer.md +++ b/content/7. Audit Findings 101/ERC20 tokens with no return value will fail to transfer.md @@ -1,7 +1,7 @@ # 17 - [ERC20 tokens with no return value will fail to transfer](./ERC20%20tokens%20with%20no%20return%20value%20will%20fail%20to%20transfer.md) -ERC20 tokens with no return value will fail to transfer Although the ERC20 standard suggests that a transfer should return true on success, many tokens are non-compliant in this regard. +Although the ERC20 standard suggests that a transfer should return true on success, many tokens are non-compliant in this regard. In that case, the `.transfer()` call here will revert even if the transfer is successful, because solidity will check that the `RETURNDATASIZE` matches the ERC20 interface. ### Recommendation: diff --git "a/content/7. Audit Findings 101/Error codes of Compound\342\200\231s `Comptroller.enterMarket`, `Comptroller.exitMarket` are not checked.md" "b/content/7. Audit Findings 101/Error codes of Compound\342\200\231s `Comptroller.enterMarket`, `Comptroller.exitMarket` are not checked.md" index 6d65f2b3..b59811fb 100644 --- "a/content/7. Audit Findings 101/Error codes of Compound\342\200\231s `Comptroller.enterMarket`, `Comptroller.exitMarket` are not checked.md" +++ "b/content/7. Audit Findings 101/Error codes of Compound\342\200\231s `Comptroller.enterMarket`, `Comptroller.exitMarket` are not checked.md" @@ -1,7 +1,10 @@ # 4 - [Error codes of Compound’s `Comptroller.enterMarket`, `Comptroller.exitMarket` are not checked](./Error%20codes%20of%20Compound’s%20`Comptroller.enterMarket`,%20`Comptroller.exitMarket`%20are%20not%20checked.md) -Error codes of Compound’s `Comptroller.enterMarket`, `Comptroller.exitMarket` are not checked Compound’s `enterMarket`/`exitMarket` functions return an error code instead of reverting in case of failure. DeFi Saver smart contracts never check for the error codes returned from Compound smart contracts. +Compound’s `enterMarket`/`exitMarket` functions return an error code instead of reverting in case of failure. + +DeFi Saver smart contracts never check for the error codes returned from Compound smart contracts. + ### Recommendation: Caller contract should revert in case the error code is not 0 ___ diff --git a/content/7. Audit Findings 101/Flash minting can be used to redeem `fyDAI`.md b/content/7. Audit Findings 101/Flash minting can be used to redeem `fyDAI`.md index 7d37fff3..a63d14cb 100644 --- a/content/7. Audit Findings 101/Flash minting can be used to redeem `fyDAI`.md +++ b/content/7. Audit Findings 101/Flash minting can be used to redeem `fyDAI`.md @@ -4,7 +4,11 @@ The flash-minting feature from the `fyDAI` token can be used to redeem an arbitrary amount of funds from a mature token. ### Recommendation: -Short term, disallow calls to redeem in the `YDai` and Unwind contracts during flash minting. Long term, do not include operations that allow any user to manipulate an arbitrary amount of funds, even if it is in a single transaction. This will prevent attackers from gaining leverage to manipulate the market and break internal invariants. +Short term, disallow calls to redeem in the `YDai` and Unwind contracts during flash minting. + +Long term, do not include operations that allow any user to manipulate an arbitrary amount of funds, even if it is in a single transaction. + +This will prevent attackers from gaining leverage to manipulate the market and break internal invariants. ___ ## Slide Screenshot ![042.png](../../images/7.%20Audit%20Findings%20101/042.png) diff --git a/content/7. Audit Findings 101/Improper Supply Cap Limitation Enforcement.md b/content/7. Audit Findings 101/Improper Supply Cap Limitation Enforcement.md index d5a81613..de1daa9c 100644 --- a/content/7. Audit Findings 101/Improper Supply Cap Limitation Enforcement.md +++ b/content/7. Audit Findings 101/Improper Supply Cap Limitation Enforcement.md @@ -1,7 +1,7 @@ # 67 - [Improper Supply Cap Limitation Enforcement](./Improper%20Supply%20Cap%20Limitation%20Enforcement.md) -Improper Supply Cap Limitation Enforcement The `openLoan()` function does not check if the loan to be issued will result in the supply cap being exceeded. +The `openLoan()` function does not check if the loan to be issued will result in the supply cap being exceeded. It only enforces that the supply cap is not reached before the loan is opened. diff --git a/content/7. Audit Findings 101/Initialization functions can be front-run.md b/content/7. Audit Findings 101/Initialization functions can be front-run.md index 97b26b97..e4e127d8 100644 --- a/content/7. Audit Findings 101/Initialization functions can be front-run.md +++ b/content/7. Audit Findings 101/Initialization functions can be front-run.md @@ -8,7 +8,11 @@ All these functions can be front-run by an attacker, allowing them to initialize ### Recommendation: Short term, either: 1. Use a factory pattern that will prevent front-running of the initialization, or - 2. Ensure the deployment scripts are robust in case of a front-running attack. Carefully review the Solidity documentation, especially the Warnings section. Carefully review the pitfalls of using delegatecall proxy pattern. + 2. Ensure the deployment scripts are robust in case of a front-running attack. + +Carefully review the Solidity documentation, especially the Warnings section. + +Carefully review the pitfalls of using delegatecall proxy pattern. ___ ## Slide Screenshot ![048.png](../../images/7.%20Audit%20Findings%20101/048.png) diff --git a/content/7. Audit Findings 101/Lack of return value checks can lead to unexpected results.md b/content/7. Audit Findings 101/Lack of return value checks can lead to unexpected results.md index 7cf86f19..93396194 100644 --- a/content/7. Audit Findings 101/Lack of return value checks can lead to unexpected results.md +++ b/content/7. Audit Findings 101/Lack of return value checks can lead to unexpected results.md @@ -1,7 +1,7 @@ # 38 - [Lack of return value checks can lead to unexpected results](./Lack%20of%20return%20value%20checks%20can%20lead%20to%20unexpected%20results.md) -Lack of return value checks can lead to unexpected results Several function calls do not check the return value. +Several function calls do not check the return value. Without a return value check, the code is error-prone, which may lead to unexpected results. diff --git a/content/7. Audit Findings 101/Lack of two-step procedure for critical operations leaves them error-prone.md b/content/7. Audit Findings 101/Lack of two-step procedure for critical operations leaves them error-prone.md index cc44003d..cbe2f5f5 100644 --- a/content/7. Audit Findings 101/Lack of two-step procedure for critical operations leaves them error-prone.md +++ b/content/7. Audit Findings 101/Lack of two-step procedure for critical operations leaves them error-prone.md @@ -12,7 +12,9 @@ If the address is incorrect, the new address will take on the functionality of t However, a two-step process is similar to the approve-transferFrom functionality: The contract approves the new address for a new role, and the new address acquires the role by calling the contract. ### Recommendation: -Short term, use a two-step procedure for all non-recoverable critical operations to prevent irrecoverable mistakes. Long term, identify and document all possible actions and their associated risks for privileged accounts. Identifying the risks will assist codebase review and prevent future mistakes. +Short term, use a two-step procedure for all non-recoverable critical operations to prevent irrecoverable mistakes. Long term, identify and document all possible actions and their associated risks for privileged accounts. + +Identifying the risks will assist codebase review and prevent future mistakes. ___ ## Slide Screenshot ![047.png](../../images/7.%20Audit%20Findings%20101/047.png) diff --git a/content/7. Audit Findings 101/Overflow-underflow protection.md b/content/7. Audit Findings 101/Overflow-underflow protection.md index 06b0090c..358529f2 100644 --- a/content/7. Audit Findings 101/Overflow-underflow protection.md +++ b/content/7. Audit Findings 101/Overflow-underflow protection.md @@ -1,7 +1,7 @@ # 14 - [Overflow-underflow protection](./Overflow-underflow%20protection.md) -Overflow-underflow protection Having overflow/underflow vulnerabilities is very common for smart contracts. It is usually mitigated by using SafeMath or using solidity version ^0.8 (after solidity 0.8 arithmetical operations already have default overflow/underflow protection). In this code, many arithmetical operations are used without the ‘safe’ version. The reasoning behind it is that all the values are derived from the actual ETH values, so they can’t overflow. +Having overflow/underflow vulnerabilities is very common for smart contracts. It is usually mitigated by using SafeMath or using solidity version ^0.8 (after solidity 0.8 arithmetical operations already have default overflow/underflow protection). In this code, many arithmetical operations are used without the ‘safe’ version. The reasoning behind it is that all the values are derived from the actual ETH values, so they can’t overflow. ### Recommendation: In our opinion, it is still safer to have these operations in a safe mode. So we recommend using SafeMath or solidity version ^0.8 compiler. ___ diff --git a/content/7. Audit Findings 101/Owner can front-run traders by updating adapters.md b/content/7. Audit Findings 101/Owner can front-run traders by updating adapters.md index 1a218f78..94aad71a 100644 --- a/content/7. Audit Findings 101/Owner can front-run traders by updating adapters.md +++ b/content/7. Audit Findings 101/Owner can front-run traders by updating adapters.md @@ -1,7 +1,7 @@ # 20 - [Owner can front-run traders by updating adapters](./Owner%20can%20front-run%20traders%20by%20updating%20adapters.md) -Owner can front-run traders by updating adapters `MetaSwap` owners can front-run users to swap an adapter implementation. +`MetaSwap` owners can front-run users to swap an adapter implementation. This could be used by a malicious or compromised owner to steal from users. diff --git a/content/7. Audit Findings 101/Proposal transactions can be executed separately and block `Proposal.execute` call.md b/content/7. Audit Findings 101/Proposal transactions can be executed separately and block `Proposal.execute` call.md index 0ac6e331..7dd2370b 100644 --- a/content/7. Audit Findings 101/Proposal transactions can be executed separately and block `Proposal.execute` call.md +++ b/content/7. Audit Findings 101/Proposal transactions can be executed separately and block `Proposal.execute` call.md @@ -1,7 +1,7 @@ # 35 - [Proposal transactions can be executed separately and block `Proposal.execute` call](./Proposal%20transactions%20can%20be%20executed%20separately%20and%20block%20`Proposal.execute`%20call.md) -Proposal transactions can be executed separately and block `Proposal.execute` call Missing access controls in the `Timelock.executeTransaction` function allow Proposal transactions to be executed separately, circumventing the `Governor.execute` function. +Missing access controls in the `Timelock.executeTransaction` function allow Proposal transactions to be executed separately, circumventing the `Governor.execute` function. ### Recommendation: Short term, only allow the admin to call `Timelock.executeTransaction` diff --git a/content/7. Audit Findings 101/Purchasing and committing still possible after launch.md b/content/7. Audit Findings 101/Purchasing and committing still possible after launch.md index 9334459f..6ceae2aa 100644 --- a/content/7. Audit Findings 101/Purchasing and committing still possible after launch.md +++ b/content/7. Audit Findings 101/Purchasing and committing still possible after launch.md @@ -1,7 +1,7 @@ # 10 - [Purchasing and committing still possible after launch](./Purchasing%20and%20committing%20still%20possible%20after%20launch.md) -Purchasing and committing still possible after launch Even after `GenesisGroup.launch` has successfully been executed, it is still possible to invoke `GenesisGroup.purchase` and `GenesisGroup.commit`. +Even after `GenesisGroup.launch` has successfully been executed, it is still possible to invoke `GenesisGroup.purchase` and `GenesisGroup.commit`. ### Recommendation: Consider adding validation in `GenesisGroup.purchase` and `GenesisGroup.commit` to make sure that these functions cannot be called after the launch. diff --git a/content/7. Audit Findings 101/Random task execution.md b/content/7. Audit Findings 101/Random task execution.md index faf531ab..1f69f65b 100644 --- a/content/7. Audit Findings 101/Random task execution.md +++ b/content/7. Audit Findings 101/Random task execution.md @@ -1,7 +1,7 @@ # 2 - [Random task execution](./Random%20task%20execution.md) -Random task execution In a scenario where a user takes a flash loan, `_parseFLAndExecute()` gives the flash loan wrapper contract (`FLAaveV2`, `FLDyDx`) the permission to execute functions on behalf of the user’s `DSProxy`. +In a scenario where a user takes a flash loan, `_parseFLAndExecute()` gives the flash loan wrapper contract (`FLAaveV2`, `FLDyDx`) the permission to execute functions on behalf of the user’s `DSProxy`. This execution permission is revoked only after the entire recipe execution is finished, which means that in case that any of the external calls along the recipe execution is malicious, it might call `executeAction()` back, i.e. Reentrancy Attack, and inject any task it wishes (e.g. take user’s funds out, drain approved tokens, etc) diff --git a/content/7. Audit Findings 101/Reentrancy vulnerability in `MetaSwap.swap()`.md b/content/7. Audit Findings 101/Reentrancy vulnerability in `MetaSwap.swap()`.md index 2682a867..7b3926be 100644 --- a/content/7. Audit Findings 101/Reentrancy vulnerability in `MetaSwap.swap()`.md +++ b/content/7. Audit Findings 101/Reentrancy vulnerability in `MetaSwap.swap()`.md @@ -1,7 +1,7 @@ # 18 - [Reentrancy vulnerability in `MetaSwap.swap()`](./Reentrancy%20vulnerability%20in%20`MetaSwap.swap()`.md) -Reentrancy vulnerability in `MetaSwap.swap()` If an attacker is able to reenter `swap()`, they can execute their own trade using the same tokens and get all the tokens for themselves. +If an attacker is able to reenter `swap()`, they can execute their own trade using the same tokens and get all the tokens for themselves. ### Recommendation: Use a simple reentrancy guard, such as OpenZeppelin’s ReentrancyGuard to prevent reentrancy in `MetaSwap.swap()` ___ diff --git a/content/7. Audit Findings 101/Reversed order of parameters in allowance function call.md b/content/7. Audit Findings 101/Reversed order of parameters in allowance function call.md index 25f3ad4e..d6f15795 100644 --- a/content/7. Audit Findings 101/Reversed order of parameters in allowance function call.md +++ b/content/7. Audit Findings 101/Reversed order of parameters in allowance function call.md @@ -1,7 +1,7 @@ # 5 - [Reversed order of parameters in allowance function call](./Reversed%20order%20of%20parameters%20in%20allowance%20function%20call.md) -Reversed order of parameters in allowance function call the parameters that are used for the allowance function call are not in the same order that is used later in the call to `safeTransferFrom`. +The parameters that are used for the allowance function call are not in the same order that is used later in the call to `safeTransferFrom`. ### Recommendation: Reverse the order of parameters in allowance function call to fit the order that is in the safeTransferFrom function call. diff --git a/content/7. Audit Findings 101/Sponsorship front-running.md b/content/7. Audit Findings 101/Sponsorship front-running.md index d9ee5415..51d880c0 100644 --- a/content/7. Audit Findings 101/Sponsorship front-running.md +++ b/content/7. Audit Findings 101/Sponsorship front-running.md @@ -12,7 +12,7 @@ ___ ![032.png](../../images/7.%20Audit%20Findings%20101/032.png) ___ ## Slide Text -- ConsenSys Audit The Lao Finding 5.4 +- ConsenSys Audit The Lao Finding 5.7 - Timing & DoS - Major Severity - Front-running diff --git a/content/7. Audit Findings 101/Summoner can steal funds using bailout.md b/content/7. Audit Findings 101/Summoner can steal funds using bailout.md index c8ae5658..9fa31226 100644 --- a/content/7. Audit Findings 101/Summoner can steal funds using bailout.md +++ b/content/7. Audit Findings 101/Summoner can steal funds using bailout.md @@ -8,7 +8,9 @@ The intention is for the summoner to transfer these funds to the kicked member a The issue here is that it requires a lot of trust to the summoner on the one hand, and requires more time to kick the member out of the LAO. ### Recommendation: -By implementing pull pattern for token transfers, kicked member won’t be able to block the ragekick and the LAO members would be able to kick anyone much quicker. There is no need to keep the `bailout` function. +By implementing pull pattern for token transfers, kicked member won’t be able to block the ragekick and the LAO members would be able to kick anyone much quicker. + +There is no need to keep the `bailout` function. ___ ## Slide Screenshot ![031.png](../../images/7.%20Audit%20Findings%20101/031.png) diff --git "a/content/7. Audit Findings 101/Swapping on zero liquidity allows for control of the pool\342\200\231s price.md" "b/content/7. Audit Findings 101/Swapping on zero liquidity allows for control of the pool\342\200\231s price.md" index 65d97208..cd5f7463 100644 --- "a/content/7. Audit Findings 101/Swapping on zero liquidity allows for control of the pool\342\200\231s price.md" +++ "b/content/7. Audit Findings 101/Swapping on zero liquidity allows for control of the pool\342\200\231s price.md" @@ -1,7 +1,7 @@ # 53 - [Swapping on zero liquidity allows for control of the pool’s price](./Swapping%20on%20zero%20liquidity%20allows%20for%20control%20of%20the%20pool’s%20price.md) -Swapping on zero liquidity allows for control of the pool’s price Swapping on a tick with zero liquidity enables a user to adjust the price of 1 wei of tokens in any direction. +Swapping on a tick with zero liquidity enables a user to adjust the price of 1 wei of tokens in any direction. As a result, an attacker could set an arbitrary price at the pool’s initialization or if the liquidity providers withdraw all of the liquidity for a short time. diff --git a/content/7. Audit Findings 101/Tokens with more than 18 decimal points will cause issues.md b/content/7. Audit Findings 101/Tokens with more than 18 decimal points will cause issues.md index 575a2701..476fdad0 100644 --- a/content/7. Audit Findings 101/Tokens with more than 18 decimal points will cause issues.md +++ b/content/7. Audit Findings 101/Tokens with more than 18 decimal points will cause issues.md @@ -1,7 +1,9 @@ # 3 - [Tokens with more than 18 decimal points will cause issues](./Tokens%20with%20more%20than%2018%20decimal%20points%20will%20cause%20issues.md) -Tokens with more than 18 decimal points will cause issues It is assumed that the maximum number of decimals for each token is 18. However uncommon, it is possible to have tokens with more than 18 decimals, as an example YAMv2 has 24 decimals. +It is assumed that the maximum number of decimals for each token is 18. + +However uncommon, it is possible to have tokens with more than 18 decimals, as an example YAMv2 has 24 decimals. This can result in broken code flow and unpredictable outcomes diff --git a/content/7. Audit Findings 101/Unchecked return value for `IWETH.transfer` call.md b/content/7. Audit Findings 101/Unchecked return value for `IWETH.transfer` call.md index 4db4271c..bb090e36 100644 --- a/content/7. Audit Findings 101/Unchecked return value for `IWETH.transfer` call.md +++ b/content/7. Audit Findings 101/Unchecked return value for `IWETH.transfer` call.md @@ -1,7 +1,7 @@ # 15 - [Unchecked return value for `IWETH.transfer` call](./Unchecked%20return%20value%20for%20`IWETH.transfer`%20call.md) -Unchecked return value for `IWETH.transfer` call In `EthUniswapPCVController`, there is a call to `IWETH.transfer` that does not check the return value. +In `EthUniswapPCVController`, there is a call to `IWETH.transfer` that does not check the return value. It is usually good to add a require-statement that checks the return value or to use something like `safeTransfer`; unless one is sure the given token reverts in case of a failure. diff --git a/content/7. Audit Findings 101/Unhandled return values of `transfer` and `transferFrom`.md b/content/7. Audit Findings 101/Unhandled return values of `transfer` and `transferFrom`.md index 9f856b01..88b79166 100644 --- a/content/7. Audit Findings 101/Unhandled return values of `transfer` and `transferFrom`.md +++ b/content/7. Audit Findings 101/Unhandled return values of `transfer` and `transferFrom`.md @@ -1,7 +1,7 @@ # 1 - [Unhandled return values of `transfer` and `transferFrom`](./Unhandled%20return%20values%20of%20`transfer`%20and%20`transferFrom`.md) -Unhandled return values of `transfer` and `transferFrom` ERC20 implementations are not always consistent. +ERC20 implementations are not always consistent. Some implementations of `transfer` and `transferFrom` could return ‘false’ on failure instead of reverting. diff --git a/content/7. Audit Findings 101/Whitelisted tokens limit.md b/content/7. Audit Findings 101/Whitelisted tokens limit.md index 9418e39a..a0650d74 100644 --- a/content/7. Audit Findings 101/Whitelisted tokens limit.md +++ b/content/7. Audit Findings 101/Whitelisted tokens limit.md @@ -1,9 +1,16 @@ # 30 - [Whitelisted tokens limit](./Whitelisted%20tokens%20limit.md) -Whitelisted tokens limit `_ragequit` function is iterating over all whitelisted tokens. If the number of tokens is too big, a transaction can run out of gas and all funds will be blocked forever. +`_ragequit` function is iterating over all whitelisted tokens. + +If the number of tokens is too big, a transaction can run out of gas and all funds will be blocked forever. + ### Recommendation: -A simple solution would be just limiting the number of whitelisted tokens. If the intention is to invest in many new tokens over time, and it’s not an option to limit the number of whitelisted tokens, it’s possible to add a function that removes tokens from the whitelist. For example, it’s possible to add a new type of proposal that is used to vote on token removal if the balance of this token is zero. Before voting for that, shareholders should sell all the balance of that token. +A simple solution would be just limiting the number of whitelisted tokens. + +If the intention is to invest in many new tokens over time, and it’s not an option to limit the number of whitelisted tokens, it’s possible to add a function that removes tokens from the whitelist. + +For example, it’s possible to add a new type of proposal that is used to vote on token removal if the balance of this token is zero. Before voting for that, shareholders should sell all the balance of that token. ___ ## Slide Screenshot ![030.png](../../images/7.%20Audit%20Findings%20101/030.png) diff --git a/content/7. Audit Findings 101/`BondingCurve` allows users to acquire FEI before launch.md b/content/7. Audit Findings 101/`BondingCurve` allows users to acquire FEI before launch.md index f93afacd..bdb2c58c 100644 --- a/content/7. Audit Findings 101/`BondingCurve` allows users to acquire FEI before launch.md +++ b/content/7. Audit Findings 101/`BondingCurve` allows users to acquire FEI before launch.md @@ -7,7 +7,6 @@ By force-sending the contract 1 wei, anyone can bypass the majority of checks an ### Recommendation: Prevent allocate from being called before genesis launch - ___ ## Slide Screenshot ![012.png](../../images/7.%20Audit%20Findings%20101/012.png) diff --git a/content/7. Audit Findings 101/`DAOfiV1Pair.deposit()` accepts deposits of zero, blocking the pool.md b/content/7. Audit Findings 101/`DAOfiV1Pair.deposit()` accepts deposits of zero, blocking the pool.md index 25cb92a7..d40b82d6 100644 --- a/content/7. Audit Findings 101/`DAOfiV1Pair.deposit()` accepts deposits of zero, blocking the pool.md +++ b/content/7. Audit Findings 101/`DAOfiV1Pair.deposit()` accepts deposits of zero, blocking the pool.md @@ -1,7 +1,7 @@ # 8 - [`DAOfiV1Pair.deposit()` accepts deposits of zero, blocking the pool](./`DAOfiV1Pair.deposit()`%20accepts%20deposits%20of%20zero,%20blocking%20the%20pool.md) -`DAOfiV1Pair.deposit()` accepts deposits of zero, blocking the pool `DAOfiV1Pair.deposit()` is used to deposit liquidity into the pool. +`DAOfiV1Pair.deposit()` is used to deposit liquidity into the pool. Only a single deposit can be made, so no liquidity can ever be added to a pool where `deposited == true`. diff --git a/content/7. Audit Findings 101/`GenesisGroup.commit` overwrites previously-committed values.md b/content/7. Audit Findings 101/`GenesisGroup.commit` overwrites previously-committed values.md index da7027a5..98c381f3 100644 --- a/content/7. Audit Findings 101/`GenesisGroup.commit` overwrites previously-committed values.md +++ b/content/7. Audit Findings 101/`GenesisGroup.commit` overwrites previously-committed values.md @@ -1,7 +1,7 @@ # 9 - [`GenesisGroup.commit` overwrites previously-committed values](./`GenesisGroup.commit`%20overwrites%20previously-committed%20values.md) -`GenesisGroup.commit` overwrites previously-committed values The amount stored in the recipient’s `committedFGEN` balance overwrites any previously-committed value. Additionally, this also allows anyone to commit an amount of “0” to any account, deleting their commitment entirely. +The amount stored in the recipient’s `committedFGEN` balance overwrites any previously-committed value. Additionally, this also allows anyone to commit an amount of “0” to any account, deleting their commitment entirely. ### Recommendation: Ensure the committed amount is added to the existing commitment. ___ diff --git a/content/7. Audit Findings 101/`GenesisGroup.emergencyExit` remains functional after launch.md b/content/7. Audit Findings 101/`GenesisGroup.emergencyExit` remains functional after launch.md index a38a4bd7..1fe395ed 100644 --- a/content/7. Audit Findings 101/`GenesisGroup.emergencyExit` remains functional after launch.md +++ b/content/7. Audit Findings 101/`GenesisGroup.emergencyExit` remains functional after launch.md @@ -1,7 +1,7 @@ # 16 - [`GenesisGroup.emergencyExit` remains functional after launch](./`GenesisGroup.emergencyExit`%20remains%20functional%20after%20launch.md) -`GenesisGroup.emergencyExit` remains functional after launch `emergencyExit` is intended as an escape mechanism for users in the event the genesis launch method fails or is frozen. `emergencyExit` becomes callable 3 days after launch is callable. These two methods are intended to be mutually-exclusive, but are not: either method remains callable after a successful call to the other. This may result in accounting edge cases. +`emergencyExit` is intended as an escape mechanism for users in the event the genesis launch method fails or is frozen. `emergencyExit` becomes callable 3 days after launch is callable. These two methods are intended to be mutually-exclusive, but are not: either method remains callable after a successful call to the other. This may result in accounting edge cases. ### Recommendation: diff --git a/content/7. Audit Findings 101/`Saferagequit` makes you lose funds.md b/content/7. Audit Findings 101/`Saferagequit` makes you lose funds.md index fb1c729d..4dbee7d3 100644 --- a/content/7. Audit Findings 101/`Saferagequit` makes you lose funds.md +++ b/content/7. Audit Findings 101/`Saferagequit` makes you lose funds.md @@ -1,7 +1,7 @@ # 26 - [`Saferagequit` makes you lose funds](./`Saferagequit`%20makes%20you%20lose%20funds.md) -`Saferagequit` makes you lose funds `safeRagequit` and `ragequit` functions are used for withdrawing funds from the LAO. The difference between them is that ragequit function tries to withdraw all the allowed tokens and `safeRagequit` function withdraws only some subset of these tokens, defined by the user. +`safeRagequit` and `ragequit` functions are used for withdrawing funds from the LAO. The difference between them is that ragequit function tries to withdraw all the allowed tokens and `safeRagequit` function withdraws only some subset of these tokens, defined by the user. It’s needed in case the user or `GuildBank` is blacklisted in some of the tokens and the transfer reverts. diff --git a/content/7. Audit Findings 101/`Timed.isTimeEnded` returns true if the timer has not been initialized.md b/content/7. Audit Findings 101/`Timed.isTimeEnded` returns true if the timer has not been initialized.md index deee42e8..0d30ab44 100644 --- a/content/7. Audit Findings 101/`Timed.isTimeEnded` returns true if the timer has not been initialized.md +++ b/content/7. Audit Findings 101/`Timed.isTimeEnded` returns true if the timer has not been initialized.md @@ -1,9 +1,11 @@ # 13 - [`Timed.isTimeEnded` returns true if the timer has not been initialized](./`Timed.isTimeEnded`%20returns%20true%20if%20the%20timer%20has%20not%20been%20initialized.md) -`Timed.isTimeEnded` returns true if the timer has not been initialized `Timed` initialization is a 2-step process: +`Timed` initialization is a 2-step process: 1. `Timed.duration` is set in the constructor - 2. `Timed.startTime` is set when the method `_initTimed` is called. Before this second method is called, `isTimeEnded()` calculates remaining time using a `startTime` of 0, resulting in the method returning true for most values, even though the timer has not technically been started. + 2. `Timed.startTime` is set when the method `_initTimed` is called. + +Before this second method is called, `isTimeEnded()` calculates remaining time using a `startTime` of 0, resulting in the method returning true for most values, even though the timer has not technically been started. ### Recommendation: If Timed has not been initialized, `isTimeEnded()` should return false, or revert diff --git a/content/7. Audit Findings 101/`UniswapIncentive` overflow on pre-transfer hooks.md b/content/7. Audit Findings 101/`UniswapIncentive` overflow on pre-transfer hooks.md index 34a9fc3c..822b1a4b 100644 --- a/content/7. Audit Findings 101/`UniswapIncentive` overflow on pre-transfer hooks.md +++ b/content/7. Audit Findings 101/`UniswapIncentive` overflow on pre-transfer hooks.md @@ -1,9 +1,11 @@ # 11 - [`UniswapIncentive` overflow on pre-transfer hooks](./`UniswapIncentive`%20overflow%20on%20pre-transfer%20hooks.md) -`UniswapIncentive` overflow on pre-transfer hooks Before a token transfer is performed, Fei performs some combination of mint/burn operations via `UniswapIncentive.incentivize`. +Before a token transfer is performed, Fei performs some combination of mint/burn operations via `UniswapIncentive.incentivize`. -Both `incentivizeBuy` and `incentivizeSell` calculate buy/sell incentives using overflow-prone math, then mint / burn from the target according to the results. This may have unintended consequences, like allowing a caller to mint tokens before transferring them, or burn tokens from their recipient. +Both `incentivizeBuy` and `incentivizeSell` calculate buy/sell incentives using overflow-prone math, then mint / burn from the target according to the results. + +This may have unintended consequences, like allowing a caller to mint tokens before transferring them, or burn tokens from their recipient. ### Recommendation: Ensure casts in `getBuyIncentive` and `getSellPenalty` do not overflow diff --git a/content/7. Audit Findings 101/`swapExactTokensForETH` checks the wrong return value.md b/content/7. Audit Findings 101/`swapExactTokensForETH` checks the wrong return value.md index 1cf6eecb..a32e5cc7 100644 --- a/content/7. Audit Findings 101/`swapExactTokensForETH` checks the wrong return value.md +++ b/content/7. Audit Findings 101/`swapExactTokensForETH` checks the wrong return value.md @@ -1,7 +1,7 @@ # 7 - [`swapExactTokensForETH` checks the wrong return value](./`swapExactTokensForETH`%20checks%20the%20wrong%20return%20value.md) -`swapExactTokensForETH` checks the wrong return value Instead of checking that the amount of tokens received from a swap is greater than the minimum amount expected from this swap, it calculates the difference between the initial receiver’s balance and the balance of the router +Instead of checking that the amount of tokens received from a swap is greater than the minimum amount expected from this swap, it calculates the difference between the initial receiver’s balance and the balance of the router ### Recommendation: Check the intended values ___