Merge pull request #135 from valory-xyz/v1.2.0-internal-audit

V1.2.0 internal audit

audits/README.md

@@ -25,6 +25,8 @@ An internal audit with a focus on `OptimismMesseger and WormholeMessenger` is lo
 
 An internal audit with a focus on `Guard for Community Multisig (CM) (modular version)` is located in this folder: [internal audit 10](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal10).
 
+An internal audit with a focus on `VoteWeighting` is located in this folder: [internal audit 10](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal12).
+
 ### External audit
 Following the initial contracts [audit report](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/Valory%20Review%20Final.pdf),
 the recommendations are addressed here: [feedback](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/Addressing%20Initial%20ApeWorX%20Recommentations.pdf).
@@ -37,4 +39,4 @@ The final audit reports:
 
 - [v3](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/Valory%20Smart%20Contract%20Audit%20by%20Solidity%20Finance-v1.1.0.pdf),
 
-- [v4](https://sourcehat.com/audits/ValoryOLAS/).
+- [v4](https://sourcehat.com/audits/ValoryOLAS/).

audits/internal12/README.md

@@ -0,0 +1,151 @@
+# autonolas-governance-audit
+The review has been performed based on the contract code in the following repository:<br>
+`https://github.com/valory-xyz/autonolas-governance` <br>
+commit: `6c39aa6dcdc0111fd8d70bb4df3433d93d4cae99` or `tag: v1.2.0-internal-audit` <br> 
+
+Update: 13-05-2024  <br>
+
+## Objectives
+The audit focused on VoteWeighting. <BR>
+
+### Flatten version
+Flatten version of contracts. [contracts](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal12/analysis/contracts)
+
+
+### Coverage
+Hardhat coverage has been performed before the audit and can be found here:
+```sh
+--------------------------------------|----------|----------|----------|----------|----------------|
+File                                  |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
+--------------------------------------|----------|----------|----------|----------|----------------|
+  VoteWeighting.sol                   |      100 |      100 |      100 |      100 |                |
+```
+
+### Fuzzing VoteWeighting
+
+#### Prepare contracts for fuzzing
+contracts/test/VoteWeightingFuzzing.sol <br>
+contracts/test/EchidnaVoteWeightingAssert.sol <br> 
+
+#### Fuzzing
+```sh
+# Move the script to the root of the project
+cp start_echidna.sh ../../../../../../
+# Move config file to the root of the project
+cp echidna_assert.yaml ../../../../../
+cd ../../../../../../
+# Run 
+./start_echidna.sh
+```
+result overflow: [fuzzing-overflow.PNG](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal12/analysis/fuzzing/overflow/fuzzing-overflow.PNG) <br>
+result assert: [fuzzing-assert.PNG](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal12/analysis/fuzzing/overflow/fuzzing-assert.PNG)
+
+
+### Security issues
+Details in [slither_full](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal12/analysis/slither_full.txt) <br>
+
+#### Issue
+Bug in viper->solidity conversion.
+```sh
+convert in viper more safe than solidity
+https://vyper.readthedocs.io/_/downloads/en/stable/pdf/
+• Converting between signed and unsigned integers reverts if the input is negative.
+bug on line:
+uint256 slope = uint256(uint128(IVEOLAS(ve).getLastUserPoint(msg.sender).slope));
+
+Proof:
+uint256 slope = uint256(uint128(IVEOLAS(ve).getLastUserPoint(msg.sender).slope));
+to
+// Hack
+pp = IVEOLAS(ve).getLastUserPoint(msg.sender).slope;
+pp = -10;
+uint256 slope = uint256(uint128(pp));
+console.log(slope);
+console.log("bug: negative getLastUserPoint() is possible");
+
+340282366920938463463374607431768211446
+bug: negative getLastUserPoint() is ok
+```
+#### Minor issue
+CEI pattern: <br>
+```sh
+Not CEI pattern. Move to end.
+        // Remove nominee in dispenser, if applicable
+        address localDispenser = dispenser;
+        if (localDispenser != address(0)) {
+            IDispenser(localDispenser).removeNominee(nomineeHash);
+        }
+
+```
+Lacks a zero-check on: <br>
+```sh
+function changeDispenser(address newDispenser) external {}
+```
+No events: <br>
+```sh
+function changeDispenser(address newDispenser) external {}
+function checkpoint() ?
+function checkpointNominee() ?
+function nomineeRelativeWeightWrite() ?
+```
+Naming test issue: <br>
+```sh
+Rename test\VoteWeighting.js
+describe("Voting Escrow OLAS", function () {
+```
+README issue: <br>
+```sh
+No link to https://github.com/curvefi/curve-dao-contracts/blob/master/contracts/GaugeController.vy
+```
+Pay attention: <br>
+```
+https://github.com/trailofbits/publications/blob/master/reviews/CurveDAO.pdf -> 18. Several loops are not executable due to gaslimitation
+Discussion: I don't think this is a problem for our version.
+```
+Version solidity: <br>
+```sh
+For contracts that are planned to be deployed in mainnet, it is necessary to use the features of the latest hard fork.
+https://soliditylang.org/blog/2024/03/14/solidity-0.8.25-release-announcement/
+```
+
+#### Notes
+Notes for UX/UI:
+```sh
+    // Remove the nominee
+    await vw.removeNominee(nominees[0], chainId);
+    // Get the removed nominee Id
+    id = await vw.getNomineeId(nominees[0], chainId);
+    expect(id).to.equal(0);
+    // Get the id for the second nominee that was shifted from 2 to 1
+    id = await vw.getNomineeId(nominees[1], chainId);
+            +
+    function getNomineeId(bytes32 account, uint256 chainId) external view returns (uint256 id) {
+        // Get the nominee struct and hash
+        Nominee memory nominee = Nominee(account, chainId);
+        bytes32 nomineeHash = keccak256(abi.encode(nominee));
+
+        id = mapNomineeIds[nomineeHash];
+    }
+    function getNominee(uint256 id) external view returns (Nominee memory nominee) {
+        // Get the total number of nominees in the contract
+        uint256 totalNumNominees = setNominees.length - 1;
+        // Check for the zero id or the overflow
+        if (id == 0) {
+            revert ZeroValue();
+        } else if (id > totalNumNominees) {
+            revert Overflow(id, totalNumNominees);
+        }
+
+        nominee = setNominees[id];
+    }
+Due to operation removeNominee(), you must keep in mind that for the same `id` there can be DIFFERENT(!) `nominee` in different time. ref: tests
+Does the developer need to add clarification in comments to the source code? 
+```
+General notes (from Curve Finance audit): <br>
+```sh
+https://github.com/trailofbits/publications/blob/master/reviews/CurveDAO.pdf
+4. GaugeController allowsfor quick vote andwithdrawvoting strategy: ref: source variable WEIGHT_VOTE_DELAY
+18. Several loops are not executable due to gaslimitation
+```
+
+