Skip to content

Commit

Permalink
updated: data sources 2024-09-17
Browse files Browse the repository at this point in the history
  • Loading branch information
automatoour committed Sep 17, 2024
1 parent 2d750f5 commit 6168683
Show file tree
Hide file tree
Showing 3 changed files with 3,800 additions and 6 deletions.
12 changes: 6 additions & 6 deletions results/chainsecurity_findings.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@
},
{
"title": "5.3 Lift Does Not Drop Unfinalized opPoke",
"body": " ScribeOptimistic generally drops unfinalized optimistic poke data after the update of parameters to avoid any issues connected to an unexpected change of the verification result. _lift() is not overridden in ScribeOptimistic to call _afterAuthedAction() which drops the unfinalized opPokeData. This may allow not yet but soon to be feeds to sign the price update. CS-CSC-003 Assume Alice is not a member of the current feeds at t and t < t < t . 2 , Alice signs a price with other bar-1 feeds, and opPoke() it. 0 0 1 At t 0 At t 1 At t 2 , wards add Alice to the feeds. pokeData becomes valid. , one comes to challenge the opPokeData, the challenge fails (verification succeeds) and the In this example, Alice's signed data successfully passes the verification, though Alice has not been authorized at t , the time the price data was aggregated. 0 Risk accepted: Chronicle states: Chronicle - Scribe - 14 DesignLowVersion1RiskAcceptedCorrectnessLowVersion1RiskAccepted \fThis is a valid issue from a theoretical point of view. However, practically we don't see any problems arising through this. ",
"body": " ScribeOptimistic generally drops unfinalized optimistic poke data after the update of parameters to avoid any issues connected to an unexpected change of the verification result. _lift() is not overridden in ScribeOptimistic to call _afterAuthedAction() which drops the unfinalized opPokeData. This may allow not yet but soon to be feeds to sign the price update. CS-CSC-003 Assume Alice is not a member of the current feeds at t and t < t < t . 2 , Alice signs a price with other bar-1 feeds, and opPoke() it. 0 1 0 At t 0 At t 1 At t 2 , wards add Alice to the feeds. pokeData becomes valid. , one comes to challenge the opPokeData, the challenge fails (verification succeeds) and the In this example, Alice's signed data successfully passes the verification, though Alice has not been authorized at t , the time the price data was aggregated. 0 Risk accepted: Chronicle states: Chronicle - Scribe - 14 DesignLowVersion1RiskAcceptedCorrectnessLowVersion1RiskAccepted \fThis is a valid issue from a theoretical point of view. However, practically we don't see any problems arising through this. ",
"labels": [
"ChainSecurity"
],
Expand Down Expand Up @@ -217,7 +217,7 @@
},
{
"title": "6.5 Gas Optimizations",
"body": " getSignerIndexLength() could load the length by assembly. In _verifySchnorrSignature(), lift(), and drop() the counter i inside of the for loop can be increased in an unchecked scope, as it is always bounded. CS-CSC-010 There is no amount limitation of inputs for abi.encodePacked(), thus one invocation should and constructPokeMessage() parameters the in suffice _constructOpPokeMessage(). pack all to In _verifySchnorrSignature(), loading a public key at an index can be abstracted into another internal function to decrease code duplication. In _lift(), the require statement index <= maxFeeds can be moved into the if branch, as we only need to check the number of feeds when a new public key is added. Chronicle - Scribe - 18 CorrectnessLowVersion1Speci\ufb01cationChangedDesignLowVersion1CodeCorrectedInformationalVersion1CodeCorrected \f The first condition check (opPokeDataFinalized) can be removed in _opPoke(), as the function would already revert if it is false. if (!opPokeDataFinalized) { revert InChallengePeriod(); } uint32 age = opPokeDataFinalized && opPokeData.age > _pokeData.age ? opPokeData.age : _pokeData.age; In LibSchnorr, the following line can be wrapped in an unchecked scope. uint s = LibSecp256k1.Q() - mulmod(challenge, pubKey.x, LibSecp256k1.Q()); In addAffinePoint(), some intermediate results can be cached to avoid computing repeatedly. For example: uint left = mulmod(addmod(z1, h, _P), addmod(z1, h, _P), _P); uint v = mulmod(x1, mulmod(4, mulmod(h, h, _P), _P), _P); uint j = mulmod(4, mulmod(h, mulmod(h, h, _P), _P), _P); In addition, the following optimizations only work if the external view functions are called by a smart contract. In feeds(), the for loop counter i can start from 1 as the public key at index 0 is an zero point. And i can be increased in an unchecked scope. In feeds(uint index), the input index can be checked towards 0 for early revert. And the public key at a specific index is not loaded by assembly as before. Code has been corrected to adopt some of the optimizations. Chronicle - Scribe - 19 \f7 Informational We utilize this section to point out informational findings that are less severe than issues. These informational issues allow us to point out more theoretical findings. Their explanation hopefully improves the overall understanding of the project's security. Furthermore, we point out findings which are unrelated to security. ",
"body": " getSignerIndexLength() could load the length by assembly. In _verifySchnorrSignature(), lift(), and drop() the counter i inside of the for loop can be increased in an unchecked scope, as it is always bounded. CS-CSC-010 There is no amount limitation of inputs for abi.encodePacked(), thus one invocation should and constructPokeMessage() parameters pack the in suffice _constructOpPokeMessage(). all to In _verifySchnorrSignature(), loading a public key at an index can be abstracted into another internal function to decrease code duplication. In _lift(), the require statement index <= maxFeeds can be moved into the if branch, as we only need to check the number of feeds when a new public key is added. Chronicle - Scribe - 18 CorrectnessLowVersion1Speci\ufb01cationChangedDesignLowVersion1CodeCorrectedInformationalVersion1CodeCorrected \f The first condition check (opPokeDataFinalized) can be removed in _opPoke(), as the function would already revert if it is false. if (!opPokeDataFinalized) { revert InChallengePeriod(); } uint32 age = opPokeDataFinalized && opPokeData.age > _pokeData.age ? opPokeData.age : _pokeData.age; In LibSchnorr, the following line can be wrapped in an unchecked scope. uint s = LibSecp256k1.Q() - mulmod(challenge, pubKey.x, LibSecp256k1.Q()); In addAffinePoint(), some intermediate results can be cached to avoid computing repeatedly. For example: uint left = mulmod(addmod(z1, h, _P), addmod(z1, h, _P), _P); uint v = mulmod(x1, mulmod(4, mulmod(h, h, _P), _P), _P); uint j = mulmod(4, mulmod(h, mulmod(h, h, _P), _P), _P); In addition, the following optimizations only work if the external view functions are called by a smart contract. In feeds(), the for loop counter i can start from 1 as the public key at index 0 is an zero point. And i can be increased in an unchecked scope. In feeds(uint index), the input index can be checked towards 0 for early revert. And the public key at a specific index is not loaded by assembly as before. Code has been corrected to adopt some of the optimizations. Chronicle - Scribe - 19 \f7 Informational We utilize this section to point out informational findings that are less severe than issues. These informational issues allow us to point out more theoretical findings. Their explanation hopefully improves the overall understanding of the project's security. Furthermore, we point out findings which are unrelated to security. ",
"labels": [
"ChainSecurity"
],
Expand Down Expand Up @@ -8641,7 +8641,7 @@
},
{
"title": "5.1 ERC165 Partially Implemented",
"body": " 0 0 0 1 CS-POLTOKEN-001 contract AccessControlEnumerable The contract PolygonEcosystemToken that inherits AccessControlEnumerable in the codebase does not extend the implementation of ERC165. ERC165 should either return true for all the interfaces the contract implements or be completely disabled. implements ERC165 the but Acknowledged: Polygon answered that: PolygonEcosystemToken is planned to only support AccessControlEnumerable interface and ERC20 Permit but it isn't industry practice for it to extend ERC165 so far. Polygon - Polygon Token (POL) - 10 DesignCriticalHighMediumLowAcknowledgedDesignLowVersion1Acknowledged \f6 Resolved Findings Here, we list findings that have been resolved during the course of the engagement. Their categories are explained in the Findings section. Below we provide a numerical overview of the identified findings, split up by their severity. -Severity Findings -Severity Findings -Severity Findings -Severity Findings Missing Input Sanitization Interfaces Are Missing Functions 0 0 0 2 ",
"body": " 0 0 0 1 CS-POLTOKEN-001 contract AccessControlEnumerable The contract PolygonEcosystemToken that inherits AccessControlEnumerable in the codebase does not extend the implementation of ERC165. ERC165 should either return true for all the interfaces the contract implements or be completely disabled. implements ERC165 but the Acknowledged: Polygon answered that: PolygonEcosystemToken is planned to only support AccessControlEnumerable interface and ERC20 Permit but it isn't industry practice for it to extend ERC165 so far. Polygon - Polygon Token (POL) - 10 DesignCriticalHighMediumLowAcknowledgedDesignLowVersion1Acknowledged \f6 Resolved Findings Here, we list findings that have been resolved during the course of the engagement. Their categories are explained in the Findings section. Below we provide a numerical overview of the identified findings, split up by their severity. -Severity Findings -Severity Findings -Severity Findings -Severity Findings Missing Input Sanitization Interfaces Are Missing Functions 0 0 0 2 ",
"labels": [
"ChainSecurity"
],
Expand Down Expand Up @@ -11585,7 +11585,7 @@
},
{
"title": "6.9 No Recovery of Accidental Token Transfers",
"body": " Possible In case an ERC-20 token other than the base tokens or collateral tokens is sent to the contract, then it cannot be recovered. Among other reasons, this might happen due to airdrops based on the base tokens or collateral tokens. A new function approveThis has been introduced to allow the governance to approve any ERC20 token to any address. Compound - Comet - 17 DesignLowVersion1CodeCorrectedDesignLowVersion1CodeCorrected \f6.10 Possible Contract Size Reductions Instead of creating an empty AssetConfig, and later returning (0, 0), the function _getPackedAsset could directly return (0, 0). The functions isBorrowCollateralized, getBorrowLiquidity, isLiquidatable and getLiquidationMargin share the same code with marginal modifications. The overlapping code could be factored out into new functions to save code size. The baseScale variable is only needed internally and is derived from decimals and can thus be defined as internal to reduce code size. The intialization of trackingSupplyIndex and trackingBorrowIndex to 0 in the initializeStorage function can be omitted. Corrected: __getPackedAsset now directly returns (0, 0) if an AssetConfig element is empty. Not corrected: Compound claims that the compiler opimizations already account for a sufficient getBorrowLiquidity, isBorrowCollateralized, in contract reduction isLiquidatable and getLiquidationMargin. size Not corrected: Compound does not want to make an exception for one variable. Corrected: trackingSupplyIndex and trackingBorrowIndex are no longer initialized to 0. ",
"body": " Possible In case an ERC-20 token other than the base tokens or collateral tokens is sent to the contract, then it cannot be recovered. Among other reasons, this might happen due to airdrops based on the base tokens or collateral tokens. A new function approveThis has been introduced to allow the governance to approve any ERC20 token to any address. Compound - Comet - 17 DesignLowVersion1CodeCorrectedDesignLowVersion1CodeCorrected \f6.10 Possible Contract Size Reductions Instead of creating an empty AssetConfig, and later returning (0, 0), the function _getPackedAsset could directly return (0, 0). The functions isBorrowCollateralized, getBorrowLiquidity, isLiquidatable and getLiquidationMargin share the same code with marginal modifications. The overlapping code could be factored out into new functions to save code size. The baseScale variable is only needed internally and is derived from decimals and can thus be defined as internal to reduce code size. The intialization of trackingSupplyIndex and trackingBorrowIndex to 0 in the initializeStorage function can be omitted. Corrected: __getPackedAsset now directly returns (0, 0) if an AssetConfig element is empty. Not corrected: Compound claims that the compiler opimizations already account for a sufficient getBorrowLiquidity, isBorrowCollateralized, contract reduction isLiquidatable and getLiquidationMargin. size in Not corrected: Compound does not want to make an exception for one variable. Corrected: trackingSupplyIndex and trackingBorrowIndex are no longer initialized to 0. ",
"labels": [
"ChainSecurity"
],
Expand Down Expand Up @@ -12393,7 +12393,7 @@
},
{
"title": "6.3 Farms Rely on Token to Checkpoint",
"body": " Farm._updateFarmingState() calls checkpoint() of an external ERC20Farmable contract. Then, the ERC20Farmable contract calls Farm.farmingCheckpoint(). However, a malicious ERC20Farmable to Farm.farmingCheckpoint(). Hence, the farm checkpoints could remain without updates. implementation purposefully could leave call the out farmingCheckpoint has been removed from the farm contracts. Hence, there is no need to call it. ",
"body": " Farm._updateFarmingState() calls checkpoint() of an external ERC20Farmable contract. Then, the ERC20Farmable contract calls Farm.farmingCheckpoint(). However, a malicious ERC20Farmable to Farm.farmingCheckpoint(). Hence, the farm checkpoints could remain without updates. implementation purposefully could leave call out the farmingCheckpoint has been removed from the farm contracts. Hence, there is no need to call it. ",
"labels": [
"ChainSecurity"
],
Expand Down Expand Up @@ -14697,7 +14697,7 @@
},
{
"title": "7.1 Users Must Add Farm if Default Farm Is",
"body": " Updated The deployed DelegatedShare contracts may not have a farm associated with them directly. If a farm is added later on, the users must either re-delegate or manually add the farm Pod on the DelegatedShare contract themselves. possible using It's DelegatedShare.remove/removeAll() but still keep delegating to this delegatee. Users must be careful and understand the consequences of their actions. remove himself default farm from user the for an to 1inch - Delegation - 13 NoteVersion1 \f",
"body": " Updated The deployed DelegatedShare contracts may not have a farm associated with them directly. If a farm is added later on, the users must either re-delegate or manually add the farm Pod on the DelegatedShare contract themselves. possible using It's DelegatedShare.remove/removeAll() but still keep delegating to this delegatee. Users must be careful and understand the consequences of their actions. remove himself default from farm user the for an to 1inch - Delegation - 13 NoteVersion1 \f",
"labels": [
"ChainSecurity"
],
Expand Down
Loading

0 comments on commit 6168683

Please sign in to comment.