llamaxyz / llama

Llama is an onchain governance and access control framework for smart contracts.
https://llama.xyz
MIT License
47 stars 5 forks source link

fix: make `_newCastCount` overflow resistant to handle nonconforming strategies #472

Closed mds1 closed 1 year ago

mds1 commented 1 year ago

Motivation:

Closes https://github.com/llamaxyz/llama/issues/448

The policy contract enforces an invariant that "sum of all user quantities for a role == that role's total quantity". However, when casting approval, the quantity used is returned from the strategy. A strategy might not respect this invariant.

With such a strategy, it's possible for casts to be blocked by reverting on overflow in the currentCount + quantity line as discussed in https://github.com/spearbit-audits/review-llama2/issues/16

The fix here is to make _newCastCount more robust.

Modifications:

To make _newCastCount more robust it now behaves as follows:

Result:

LlamaCore can handle strategies that don't respect the expected behavior of quantities

github-actions[bot] commented 1 year ago

Coverage after merging fix/handle-nonconforming-strategy-quantities into main will be

84.03%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   LlamaCore.sol99.26%97.30%100%100%292, 393
   LlamaExecutor.sol80%50%100%100%33
   LlamaFactory.sol100%100%100%100%
   LlamaLens.sol72.09%20%100%82.61%175–176, 179, 179, 179–180, 180, 180–181, 181, 181, 189
   LlamaPolicy.sol91.49%88.89%94.59%91.30%324, 361, 361, 361–363, 363, 363, 365–368, 370, 383
   LlamaPolicyMetadata.sol100%100%100%100%
src/accounts
   LlamaAccount.sol100%100%100%100%
src/lib
   ERC721NonTransferableMinimalProxy.sol70.42%72.73%72.73%68.42%102, 104, 106, 118–120, 190, 88, 88, 88, 90, 90, 90, 92, 92, 92, 97, 99
   LlamaUtils.sol100%100%100%100%
   PolicyholderCheckpoints.sol55.88%50%81.82%53.62%125, 179–181, 181, 181–182, 184, 187, 217, 224, 224–226, 228, 228–230, 232, 232–234, 236, 236–238, 257, 260–266, 273, 46, 46–48, 48, 48–49, 51
   SupplyCheckpoints.sol57.14%50%83.33%54.93%131, 183–185, 185, 185–186, 188, 191, 235, 242, 242–244, 246, 246–248, 250, 250–252, 254, 254–256, 275, 278–284, 291, 50, 50–52, 52, 52–53, 55
src/llama-scripts
   LlamaGovernanceScript.sol50%33.33%43.75%53.45%106–108, 115–117, 125–128, 135–137, 146–150, 157–158, 166–168, 67, 74, 74, 76, 89–90, 97–98
src/strategies/absolute
   LlamaAbsolutePeerReview.sol100%100%100%100%
   LlamaAbsoluteQuorum.sol100%100%100%100%
   LlamaAbsoluteStrategyBase.sol94.74%87.50%90.91%97.96%238, 241, 289
src/strategies/relative
   LlamaRelativeHolderQuorum.sol91.67%75%100%100%46, 53
   LlamaRelativeQuantityQuorum.sol0%0%0%0%26, 26, 26–28, 38, 38, 38–40, 45–46, 46, 46–47, 52–53, 53, 53–54
   LlamaRelativeStrategyBase.sol97.59%90.91%100%100%203, 303
   LlamaRelativeUniqueHolderQuorum.sol93.33%83.33%100%100%48, 55