Skip to content

Commit

Permalink
fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
0xTaylor committed Nov 24, 2021
1 parent 55a9d07 commit 06af1a3
Show file tree
Hide file tree
Showing 35 changed files with 65 additions and 38 deletions.
2 changes: 1 addition & 1 deletion 7. Audit Findings 101.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
Original file line number Diff line number Diff line change
@@ -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`.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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:
Expand Down
Original file line number Diff line number Diff line change
@@ -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
___
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
___
Expand Down
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
Original file line number Diff line number Diff line change
@@ -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`
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
2 changes: 1 addition & 1 deletion content/7. Audit Findings 101/Random task execution.md
Original file line number Diff line number Diff line change
@@ -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)

Expand Down
Original file line number Diff line number Diff line change
@@ -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()`
___
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
2 changes: 1 addition & 1 deletion content/7. Audit Findings 101/Sponsorship front-running.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
Original file line number Diff line number Diff line change
@@ -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

Expand Down
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
11 changes: 9 additions & 2 deletions content/7. Audit Findings 101/Whitelisted tokens limit.md
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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`.

Expand Down
Original file line number Diff line number Diff line change
@@ -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.
___
Expand Down
Loading

0 comments on commit 06af1a3

Please sign in to comment.