Skip to content

Commit

Permalink
fixes and cleanup on slot 7
Browse files Browse the repository at this point in the history
  • Loading branch information
0xTaylor committed Nov 24, 2021
1 parent 6b4bd2f commit 887aa24
Show file tree
Hide file tree
Showing 59 changed files with 549 additions and 230 deletions.
Original file line number Diff line number Diff line change
@@ -1,19 +1,23 @@

# 83 - [Adding new variables to multi-level inherited upgradeable contracts may break storage layout](./Adding%20new%20variables%20to%20multi-level%20inherited%20upgradeable%20contracts%20may%20break%20storage%20layout.md)

Adding new variables to multi-level inherited upgradeable contracts may break storage layout The Notional protocol uses the OpenZeppelin/SDK contracts to manage upgradeability in the system, which follows the unstructured storage pattern. When using this upgradeability approach, and when working with multi-level inheritance, if a new variable is introduced in a parent contract, that addition can potentially overwrite the beginning of the storage layout of the child contract, causing critical misbehaviors in the system.
The Notional protocol uses the OpenZeppelin/SDK contracts to manage upgradeability in the system, which follows the unstructured storage pattern.

When using this upgradeability approach, and when working with multi-level inheritance, if a new variable is introduced in a parent contract, that addition can potentially overwrite the beginning of the storage layout of the child contract, causing critical misbehaviors in the system.

1. Recommendation: consider preventing these scenarios by defining a storage gap in each upgradeable parent contract at the end of all the storage variable definitions as follows: `uint256[50] `_gap; // gap to reserve storage in the contract for future variable additions.` In such an implementation, the size of the gap would be intentionally decreased each time a new variable was introduced, thereby avoiding overwriting preexisting storage values.
1. Recommendation: consider preventing these scenarios by defining a storage gap in each upgradeable parent contract at the end of all the storage variable definitions as follows: `uint256[50] _gap; // gap to reserve storage in the contract for future variable additions.` In such an implementation, the size of the gap would be intentionally decreased each time a new variable was introduced, thereby avoiding overwriting preexisting storage values.
2. Medium Risk severity finding from [OpenZeppelin’s Audit of Notional Protocol](https://blog.openzeppelin.com/notional-audit/)


___
## Slide Screenshot
![083.png](../../images/7.%20Audit%20Findings%20101/083.png)
___
## Slide Text
-
- OpenZeppelin Audit National Finding M02
- Configuration
- Medium Severity
- New Variables Inherited/Upgradeable
- Add Gap
- Reserve Storage
___
## References
- Youtube Reference
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,28 @@

# 97 - [Anyone can liquidate on behalf of another account](./Anyone%20can%20liquidate%20on%20behalf%20of%20another%20account.md)

Anyone can liquidate on behalf of another account The Perpetual contract has a public liquidateFrom function that bypasses the checks in the liquidate function. This means that it can be called to liquidate a position when the contract is in the `SETTLED` state. Additionally, any user can set an arbitrary from address, causing a third-party user to confiscate the under-collateralized trader’s position. This means that any trader can unilaterally rearrange another account’s position. They could also liquidate on behalf of the Perpetual Proxy, which could break some of the Automated Market Maker invariants, such as the condition that it only holds LONG positions.
The Perpetual contract has a public liquidateFrom function that bypasses the checks in the liquidate function.

This means that it can be called to liquidate a position when the contract is in the `SETTLED` state.

1. Recommendation: Consider restricting `liquidateFrom` to internal visibility
2. Critical Risk severity finding from [OpenZeppelin’s Audit of MCDEX Mai Protocol](https://blog.openzeppelin.com/mcdex-mai-protocol-audit/)
Additionally, any user can set an arbitrary from address, causing a third-party user to confiscate the under-collateralized trader’s position.

This means that any trader can unilaterally rearrange another account’s position.

They could also liquidate on behalf of the Perpetual Proxy, which could break some of the Automated Market Maker invariants, such as the condition that it only holds LONG positions.

1. Recommendation: Consider restricting `liquidateFrom` to internal visibility
2. Critical Risk severity finding from [OpenZeppelin’s Audit of MCDEX Mai Protocol](https://blog.openzeppelin.com/mcdex-mai-protocol-audit/)
___
## Slide Screenshot
![097.png](../../images/7.%20Audit%20Findings%20101/097.png)
___
## Slide Text
-
- OpenZeppelin Audit MCDEX Mai Finding C01
- Access Control
- Critical Severity
- liquidateFrom -> Public Visibility
- Change Visibility Public -> Internal
___
## References
- Youtube Reference
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,28 @@

# 58 - [Assimilators use a deprecated Chainlink API](./Assimilators%20use%20a%20deprecated%20Chainlink%20API.md)

Assimilators use a deprecated Chainlink API The old version of the Chainlink price feed API (`AggregatorInterface`) is used throughout the contracts and tests. For example, the deprecated function `latestAnswer` is used. This function is not present in the latest API reference (`AggregatorInterfaceV3`). However, it is present in the deprecated API reference. In the worst-case scenario, the deprecated contract could cease to report the latest values, which would very likely cause liquidity providers to incur losses.
The old version of the Chainlink price feed API (`AggregatorInterface`) is used throughout the contracts and tests.

For example, the deprecated function `latestAnswer` is used.

1. Recommendation: Use the latest stable versions of any external libraries or contracts leveraged by the codebase
2. Undetermined Risk severity finding from [ToB’s Audit of DFX Finance](https://github.com/dfx-finance/protocol/blob/main/audits/2021-05-03-Trail_of_Bits.pdf)
This function is not present in the latest API reference (`AggregatorInterfaceV3`).

However, it is present in the deprecated API reference.

In the worst-case scenario, the deprecated contract could cease to report the latest values, which would very likely cause liquidity providers to incur losses.

1. Recommendation: Use the latest stable versions of any external libraries or contracts leveraged by the codebase
2. Undetermined Risk severity finding from [ToB’s Audit of DFX Finance](https://github.com/dfx-finance/protocol/blob/main/audits/2021-05-03-Trail_of_Bits.pdf)
___
## Slide Screenshot
![058.png](../../images/7.%20Audit%20Findings%20101/058.png)
___
## Slide Text
-
- ToB Audit DFX Finding 1
- Patching
- Undetermined Severity
- Deprecated Chainlink API
- Use Latest Versions of Dependencies
___
## References
- Youtube Reference
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,26 @@

# 56 - [Assimilators’ balance functions return raw values](./Assimilators’%20balance%20functions%20return%20raw%20values.md)

Assimilators’ balance functions return raw values The system converts raw values to numeraire values for its internal arithmetic. However, in one instance it uses raw values alongside numeraire values. Interchanging raw and numeraire values will produce unwanted results and may result in loss of funds for liquidity provider.
The system converts raw values to numeraire values for its internal arithmetic.

However, in one instance it uses raw values alongside numeraire values.

1. Recommendation: Short term, change the semantics of the three functions listed above in the CADC, XSGD, and EURS assimilators to return the numeraire balance. Long term, use unit tests and fuzzing to ensure that all calculations return the expected values. Additionally, ensure that changes to the Shell Protocol do not introduce bugs such as this one.
2. High Risk severity finding from [ToB’s Audit of DFX Finance](https://github.com/dfx-finance/protocol/blob/main/audits/2021-05-03-Trail_of_Bits.pdf)

Interchanging raw and numeraire values will produce unwanted results and may result in loss of funds for liquidity provider.

Recommendation: Short term, change the semantics of the three functions listed above in the CADC, XSGD, and EURS assimilators to return the numeraire balance. Long term, use unit tests and fuzzing to ensure that all calculations return the expected values. Additionally, ensure that changes to the Shell Protocol do not introduce bugs such as this one.
2. High Risk severity finding from [ToB’s Audit of DFX Finance](https://github.com/dfx-finance/protocol/blob/main/audits/2021-05-03-Trail_of_Bits.pdf)
___
## Slide Screenshot
![056.png](../../images/7.%20Audit%20Findings%20101/056.png)
___
## Slide Text
-
- ToB Audit DFX Finding 12
- Undefined Behavior
- High Severity
- Raw vs Numeraire
- Missed Conversion
- Consistent Values
- Unit Tests & Fuzzing
___
## References
- Youtube Reference
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,26 @@

# 80 - [Attackers can prevent honest users from performing an instant withdraw from the Wallet contract](./Attackers%20can%20prevent%20honest%20users%20from%20performing%20an%20instant%20withdraw%20from%20the%20Wallet%20contract.md)

Attackers can prevent honest users from performing an instant withdraw from the Wallet contract An attacker who sees an honest user’s call to `MessageProcessor.instantWithdraw` in the mempool can grab the `oracleMessage` and `oracleSignature` parameters from the user’s transaction, then submit their own transaction to `instantWithdraw` using the same parameters, a higher gas price (so as to frontrun the honest user’s transaction), and carefully choosing the gas limit for their transactions such that the internal call to the `callInstantWithdraw` will fail on line 785 with an out-of-gas error, but will successfully execute the `if(!success)` block. The result is that the attacker’s instant withdraw will fail (so the user will not receive their funds), but the `userInteractionNumber` will be successfully reserved by the `ReplayTracker`. As a result, the honest user’s transaction will revert because it will be attempting to use a `userInteractionNumber` that is no longer valid.
An attacker who sees an honest user’s call to `MessageProcessor.instantWithdraw` in the mempool can grab the `oracleMessage` and `oracleSignature` parameters from the user’s transaction, then submit their own transaction to `instantWithdraw` using the same parameters, a higher gas price (so as to frontrun the honest user’s transaction), and carefully choosing the gas limit for their transactions such that the internal call to the `callInstantWithdraw` will fail on line 785 with an out-of-gas error, but will successfully execute the `if(!success)` block.

The result is that the attacker’s instant withdraw will fail (so the user will not receive their funds), but the `userInteractionNumber` will be successfully reserved by the `ReplayTracker`.

As a result, the honest user’s transaction will revert because it will be attempting to use a `userInteractionNumber` that is no longer valid.

1. Recommendation: Consider adding an access control mechanism to restrict who can submit `oracleMessages` on behalf of the user.
2. High Risk severity finding from [OpenZeppelin’s Audit of Futureswap V2](https://blog.openzeppelin.com/futureswap-v2-audit/)


___
## Slide Screenshot
![080.png](../../images/7.%20Audit%20Findings%20101/080.png)
___
## Slide Text
-
- OpenZeppelin Audit Futureswap V2 Finding H01
- Timing/DoS
- High Severity
- Front-running
- Prevent Instant Withdraw
- Access Control
- Submit oracleMessages
___
## References
- Youtube Reference
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,28 @@

# 64 - [Batch processing of transaction execution and order matching may lead to exchange griefing](./Batch%20processing%20of%20transaction%20execution%20and%20order%20matching%20may%20lead%20to%20exchange%20griefing.md)

Batch processing of transaction execution and order matching may lead to exchange griefing Batch processing of transaction execution and order matching will iteratively process every transaction and order, which all involve filling. If the asset being filled does not have enough allowance, the asset’s transferFrom will fail, causing `AssetProxyDispatcher` to revert. NoThrow variants of batch processing, which are available for filling orders, are not available for transaction execution and order matching. So if one transaction or order fails this way, the entire batch will revert and will have to be re-submitted after the reverting transaction is removed.
Batch processing of transaction execution and order matching may lead to exchange griefing Batch processing of transaction execution and order matching will iteratively process every transaction and order, which all involve filling.

If the asset being filled does not have enough allowance, the asset’s transferFrom will fail, causing `AssetProxyDispatcher` to revert.

1. Recommendation: Short term, implement NoThrow variants for batch processing of transaction execution and order matching. Long term, take into consideration the effect of malicious inputs when implementing functions that perform a batch of operations.
2. Medium Risk severity finding from [ToB’s Audit of 0x Protocol](https://github.com/trailofbits/publications/blob/master/reviews/0x-protocol.pdf)
NoThrow variants of batch processing, which are available for filling orders, are not available for transaction execution and order matching.

So if one transaction or order fails this way, the entire batch will revert and will have to be re-submitted after the reverting transaction is removed.

1. Recommendation: Short term, implement NoThrow variants for batch processing of transaction execution and order matching. Long term, take into consideration the effect of malicious inputs when implementing functions that perform a batch of operations.
2. Medium Risk severity finding from [ToB’s Audit of 0x Protocol](https://github.com/trailofbits/publications/blob/master/reviews/0x-protocol.pdf)
___
## Slide Screenshot
![064.png](../../images/7.%20Audit%20Findings%20101/064.png)
___
## Slide Text
-
- ToB Audit Ox Protocol Finding 3
- DoS
- Medium Severity
- Batch tx Processing
- One Revert -> Batch Revert
- Nothrow Variant
- Evaluate Batch Risk
___
## References
- Youtube Reference
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,23 @@

# 71 - [Blacklisting Bypass via `transferFrom()` Function](./Blacklisting%20Bypass%20via%20`transferFrom()`%20Function.md)

Blacklisting Bypass via `transferFrom()` Function The `transferFrom()` function in the `TokenImpl` contract does not verify that the sender (i.e. the from address) is not blacklisted. As such, it is possible for a user to allow an account to spend a certain allowance regardless of their blacklisting status.
Function The `transferFrom()` function in the `TokenImpl` contract does not verify that the sender (i.e. the from address) is not blacklisted.

As such, it is possible for a user to allow an account to spend a certain allowance regardless of their blacklisting status.

1. Recommendation: At present the function `transferFrom()` uses the `notBlacklisted(address)` modifier twice, on the msg.sender and to addresses. The `notBlacklisted(address)` modifier should be used a third time against the from address.
2. High Risk severity finding from [Sigma Prime's Audit of InfiniGold](https://github.com/sigp/public-audits/raw/master/infinigold/review.pdf)


___
## Slide Screenshot
![071.png](../../images/7.%20Audit%20Findings%20101/071.png)
___
## Slide Text
-
- Sigma Prime Infinigold Finding 2
- Access Control
- High Severity
- Missed Blacklisting
- transferFrom() from Addr
- Apply notBlacklisted() from Addr
___
## References
- Youtube Reference
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,24 @@

# 66 - [Calls to `setParams` may set invalid values and produce unexpected behavior in the staking contracts](./Calls%20to%20`setParams`%20may%20set%20invalid%20values%20and%20produce%20unexpected%20behavior%20in%20the%20staking%20contracts.md)

Calls to `setParams` may set invalid values and produce unexpected behavior in the staking contracts Certain parameters of the contracts can be configured to invalid values, causing a variety of issues and breaking expected interactions between contracts. `setParams` allows the owner of the staking contracts to reparameterize critical parameters. However, reparameterization lacks sanity/threshold/limit checks on all parameters.
CCertain parameters of the contracts can be configured to invalid values, causing a variety of issues and breaking expected interactions between contracts.

`setParams` allows the owner of the staking contracts to reparameterize critical parameters. However, reparameterization lacks sanity/threshold/limit checks on all parameters.

1. Recommendation: Add proper validation checks on all parameters in `setParams_. If the validation procedure is unclear or too complex to implement on-chain, document the potential issues that could produce invalid values.
2. Medium Risk severity finding from [ToB’s Audit of 0x Protocol](https://github.com/trailofbits/publications/blob/master/reviews/0x-protocol.pdf)


___
## Slide Screenshot
![066.png](../../images/7.%20Audit%20Findings%20101/066.png)
___
## Slide Text
-
- ToB Audit Ox Protocol Finding 21
- Data Validation
- Medium Severity
- No setParams Validation
- Undefined Behavior
- Add Validation
- Document Behavior
___
## References
- Youtube Reference
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,23 @@

# 69 - [Contract Owner Can Arbitrarily Change Minting Fees and Interest Rates](./Contract%20Owner%20Can%20Arbitrarily%20Change%20Minting%20Fees%20and%20Interest%20Rates.md)

Contract Owner Can Arbitrarily Change Minting Fees and Interest Rates The `issueFeeRate` and `interestRate` variables can both be changed by the `EtherCollateral` contract owner after loans have been opened. As a result, the owner can control fees such as they equal/exceed the collateral for any given loan.
The `issueFeeRate` and `interestRate` variables can both be changed by the `EtherCollateral` contract owner after loans have been opened.

As a result, the owner can control fees such as they equal/exceed the collateral for any given loan.

1. Recommendation: While "dynamic" interest rates are common, we recommend considering the minting fee ( `issueFeeRate` ) to be a constant that cannot be changed by the owner.
2. Medium Risk severity finding from [Sigma Prime's Audit of Synthetix EtherCollateral](https://github.com/sigp/public-audits/blob/master/synthetix/ethercollateral/review.pdf)


___
## Slide Screenshot
![069.png](../../images/7.%20Audit%20Findings%20101/069.png)
___
## Slide Text
-
- Sigma Prime EtherCollateral Finding 3
- Configuration
- Medium Severity
- Contract Owner
- Change Fees & Interest
- Prevent Changes
___
## References
- Youtube Reference
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
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.

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.


1. Recommendation: Consider using OpenZeppelin’s SafeERC20
2. Major severity finding from [Consensys Diligence Audit of bitbank](https://consensys.net/diligence/audits/2020/11/bitbank/#erc20-tokens-with-no-return-value-will-fail-to-transfer)
___
Expand Down
Loading

0 comments on commit 887aa24

Please sign in to comment.