Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: C4R issue 64 #145

Merged
merged 20 commits into from
Jul 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitleaksignore
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,4 @@ ac5929932dad792efef12b2860753ca90bc4b3de:scripts/proposals/proposal_04_CM_guard_
c91f775ec4afeb1737af25b8325201c090d48c23:scripts/deployment/globals_mainnet.json:generic-api-key:1
52a1209b5f91ae48f0c44a0ca968f69a6461d6a8:scripts/proposals/proposal_04_CM_guard_goerli.js:generic-api-key:14
52a1209b5f91ae48f0c44a0ca968f69a6461d6a8:scripts/proposals/proposal_04_CM_guard_goerli.js:generic-api-key:18
fa17ac186753911eb9d2ae0a4ab59db5f7e8e563:scripts/deployment/bridges/solana/test/sepolia/timelock_set_upgrade_authority.js:generic-api-key:40
41 changes: 39 additions & 2 deletions abis/0.8.25/VoteWeighting.json

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion audits/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ 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).
An internal audit with a focus on `VoteWeighting` is located in this folder: [internal audit 12](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal12).

An internal audit with a focus on `VoteWeighting` (after C4A external audit) is located in this folder: [internal audit 13](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal13).

### External audit
Following the initial contracts [audit report](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/Valory%20Review%20Final.pdf),
Expand Down
45 changes: 45 additions & 0 deletions audits/internal13/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# 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: `59aa1c8732397c826bb67fc567b81b8d0cd82b00` or `tag: v1.2.2-pre-internal-audi` <br>

Update: 05-07-2024 <br>

## Objectives
The audit focused on fixing VoteWeighting after C4A external audit. <BR>

### 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 | 98.94 | 100 | 99.56 | 484 |

int128 userSlope = IVEOLAS(ve).getLastUserPoint(msg.sender).slope;
if (userSlope < 0) {
revert NegativeSlope(msg.sender, userSlope);
}
The fact that this case is not covered is not a problem, since it is very difficult to create such conditions in a real test.
```
#### Checking the corrections made after C4A
64. Less active nominees can be left without rewards after an year of inactivity #64
https://github.com/code-423n4/2024-05-olas-findings/issues/64 <br>
[x] fixed

36. pointsSum.slope Not Updated After Nominee Removal and Votes Revocation #36
https://github.com/code-423n4/2024-05-olas-findings/issues/36 <br>
[x] fixed

16. Incorrect Handling of Last Nominee Removal in removeNominee Function #16
https://github.com/code-423n4/2024-05-olas-findings/issues/16 <br>
[x] fixed

#### Low issue
QA Report #109
https://github.com/code-423n4/2024-05-olas-findings/issues/109
```
Lack of event emission for important state changes in revokeRemovedNomineeVotingPower()
```
[x] fixed

57 changes: 34 additions & 23 deletions contracts/VoteWeighting.sol
Original file line number Diff line number Diff line change
Expand Up @@ -137,28 +137,29 @@
uint256 nomineeWeight, uint256 totalSum);
event NomineeRelativeWeightWrite(address indexed sender, bytes32 indexed nomineeAccount, uint256 chainId,
uint256 nomineeWeight, uint256 totalSum, uint256 relativeWeight);
event VoteForNominee(address indexed user, bytes32 indexed nominee, uint256 chainId, uint256 weight);
event VoteForNominee(address indexed user, bytes32 indexed nominee, uint256 chainId, uint256 weight,
uint256 powerUsed);
event AddNominee(bytes32 indexed account, uint256 chainId, uint256 id);
event RemoveNominee(bytes32 indexed account, uint256 chainId, uint256 newSum);
event VotingPowerRevoked(address indexed user, bytes32 indexed nominee, uint256 chainId, uint256 powerUsed);

// 7 * 86400 seconds - all future times are rounded by week
uint256 public constant WEEK = 604_800;
// Cannot change weight votes more often than once in 10 days
// For explanation about the delay consult the official audit report: https://github.com/trailofbits/publications/blob/master/reviews/CurveDAO.pdf
uint256 public constant WEIGHT_VOTE_DELAY = 864_000;
// Max number of weeks for checkpoints
// The number corresponds to slightly more than a year time, that is more than enough to have at least one vote
// Also, in line with our tokenomics that cannot have epochs longer than a year
// The suggested maximum amount of weeks results in checkpoint calculation that always fit in the block,
// although in practice it is unlikely that there is no single checkpoint for the maximum amount of weeks
// The number corresponds to more than four years timeframe
// It is enough to have at least one vote while veOLAS value is greater than zero
// In practice it is unlikely that there is no single checkpoint for the maximum amount of weeks
// For gas concerns regarding checkpoint calculations, see the internal audit and the official audit report: https://github.com/trailofbits/publications/blob/master/reviews/CurveDAO.pdf
uint256 public constant MAX_NUM_WEEKS = 53;
uint256 public constant MAX_NUM_WEEKS = 250;
// Max weight amount
uint256 public constant MAX_WEIGHT = 10_000;
// Maximum chain Id as per EVM specs
uint256 public constant MAX_EVM_CHAIN_ID = type(uint64).max / 2 - 36;
// veOLAS contract address
address public immutable ve;

Check warning on line 162 in contracts/VoteWeighting.sol

View workflow job for this annotation

GitHub Actions / build

Immutable variables name are set to be in capitalized SNAKE_CASE
// Contract owner address
address public owner;
// Dispenser contract
Expand All @@ -173,31 +174,31 @@
// Mapping of hash(Nominee struct) => removed nominee Id
mapping(bytes32 => uint256) public mapRemovedNominees;

// user -> hash(Nominee struct) -> VotedSlope
// Mapping for user => hash(Nominee struct) => VotedSlope
mapping(address => mapping(bytes32 => VotedSlope)) public voteUserSlopes;
// Total vote power used by user
// Mapping for user address => total vote power used
mapping(address => uint256) public voteUserPower;
// Last user vote's timestamp for each hash(Nominee struct)
// Mapping for user address => hash(Nominee struct) => last vote timestamp
mapping(address => mapping(bytes32 => uint256)) public lastUserVote;

// Past and scheduled points for nominee weight, sum of weights per type, total weight
// Point is for bias+slope
// changes_* are for changes in slope
// time_* are for the last change timestamp
// Past and scheduled points for nominee weight, sum of weights
// Point is for bias + slope
// changes* are for changes in slope
// time* are for the last change timestamp
// timestamps are rounded to whole weeks

// hash(Nominee struct) -> time -> Point
// Mapping for hash(Nominee struct) => time => Point
mapping(bytes32 => mapping(uint256 => Point)) public pointsWeight;
// hash(Nominee struct) -> time -> slope
// Mapping for hash(Nominee struct) => time => slope
mapping(bytes32 => mapping(uint256 => uint256)) public changesWeight;
// hash(Nominee struct) -> last scheduled time (next week)
// Mapping for hash(Nominee struct) => last scheduled time (next week)
mapping(bytes32 => uint256) public timeWeight;

// time -> Point
// Mapping for time => Point
mapping(uint256 => Point) public pointsSum;
// time -> slope
// Mapping for time => slope
mapping(uint256 => uint256) public changesSum;
// last scheduled time (next week)
// Last scheduled time (next week)
uint256 public timeSum;

/// @dev Contract constructor.
Expand All @@ -218,7 +219,7 @@
setRemovedNominees.push(Nominee(0, 0));
}

/// @dev Fill sum of nominee weights for the same type week-over-week for missed checkins and return the sum for the future week.
/// @dev Fill sum of nominee weights week-over-week for missed checkins and return the sum for the future week.
/// @return Sum of nominee weights.
function _getSum() internal returns (uint256) {
// t is always > 0 as it is set in the constructor
Expand Down Expand Up @@ -469,7 +470,7 @@
/// @dev Allocates voting power for changing pool weights.
/// @param account Address of the nominee the `msg.sender` votes for in bytes32 form.
/// @param chainId Chain Id.
/// @param weight Weight for a nominee in bps (units of 0.01%). Minimal is 0.01%. Ignored if 0.
/// @param weight Weight for a nominee in bps (units of 0.01%). Minimal step is is 0.01% (1 out of 10_000).
function voteForNomineeWeights(bytes32 account, uint256 chainId, uint256 weight) public {
// Get the nominee hash
bytes32 nomineeHash = keccak256(abi.encode(Nominee(account, chainId)));
Expand All @@ -493,7 +494,7 @@
revert LockExpired(msg.sender, lockEnd, nextTime);
}

// Check for the weight number
// Check for the weight value
if (weight > MAX_WEIGHT) {
revert Overflow(weight, MAX_WEIGHT);
}
Expand Down Expand Up @@ -553,7 +554,7 @@
// Record last action time
lastUserVote[msg.sender][nomineeHash] = block.timestamp;

emit VoteForNominee(msg.sender, account, chainId, weight);
emit VoteForNominee(msg.sender, account, chainId, weight, powerUsed);
}

/// @dev Allocates voting power for changing pool weights in a batch set.
Expand Down Expand Up @@ -659,6 +660,14 @@
revert ZeroValue();
}

uint256 nextTime = (block.timestamp + WEEK) / WEEK * WEEK;
// Adjust weight and sum slope changes
if (oldSlope.end > nextTime) {
pointsWeight[nomineeHash][nextTime].slope =
_maxAndSub(pointsWeight[nomineeHash][nextTime].slope, oldSlope.slope);
pointsSum[nextTime].slope = _maxAndSub(pointsSum[nextTime].slope, oldSlope.slope);
}

// Cancel old slope changes if they still didn't happen
if (oldSlope.end > block.timestamp) {
changesWeight[nomineeHash][oldSlope.end] -= oldSlope.slope;
Expand All @@ -670,6 +679,8 @@
powerUsed = powerUsed - oldSlope.power;
voteUserPower[msg.sender] = powerUsed;
delete voteUserSlopes[msg.sender][nomineeHash];

emit VotingPowerRevoked(msg.sender, account, chainId, powerUsed);
}

/// @dev Get current nominee weight.
Expand Down
Binary file modified docs/Vulnerabilities_list_governance.pdf
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/*global process*/

const { ethers } = require("hardhat");
const { LedgerSigner } = require("@anders-t/ethers-ledger");

async function main() {
const fs = require("fs");
Expand All @@ -24,7 +23,7 @@
networkURL += process.env.ALCHEMY_API_KEY_AMOY;
}

const provider = new ethers.providers.JsonRpcProvider(networkURL);

Check warning on line 26 in scripts/deployment/bridges/solana/test/polygon/deploy_00_mock_timelock.js

View workflow job for this annotation

GitHub Actions / build

'provider' is assigned a value but never used
const signers = await ethers.getSigners();

// EOA address
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ async function main() {
const mockTimelockAddress = "0x4cEB52802ef86edF8796632546d89e55c87a0901";
const mockTimelockJSON = "artifacts/contracts/bridges/test/MockTimelock.sol/MockTimelock.json";
contractFromJSON = fs.readFileSync(mockTimelockJSON, "utf8");
parsedFile = JSON.parse(contractFromJSON);
const parsedFile = JSON.parse(contractFromJSON);
const mockTimelockABI = parsedFile["abi"];
const mockTimelock = new ethers.Contract(mockTimelockAddress, mockTimelockABI, polygonProvider);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/*global process*/

const { ethers } = require("hardhat");
const { LedgerSigner } = require("@anders-t/ethers-ledger");

async function main() {
const fs = require("fs");
Expand All @@ -10,7 +9,7 @@
let parsedData = JSON.parse(dataFromJSON);
const providerName = parsedData.providerName;

const provider = await ethers.providers.getDefaultProvider(providerName);

Check warning on line 12 in scripts/deployment/bridges/solana/test/sepolia/deploy_00_mock_timelock.js

View workflow job for this annotation

GitHub Actions / build

'provider' is assigned a value but never used
const signers = await ethers.getSigners();

// EOA address
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ async function main() {
const mockTimelockAddress = "0x471B3f60f08C50dd0eCba1bCd113B66FCC02b63d";
const mockTimelockJSON = "artifacts/contracts/bridges/test/MockTimelock.sol/MockTimelock.json";
contractFromJSON = fs.readFileSync(mockTimelockJSON, "utf8");
parsedFile = JSON.parse(contractFromJSON);
const parsedFile = JSON.parse(contractFromJSON);
const mockTimelockABI = parsedFile["abi"];
const mockTimelock = new ethers.Contract(mockTimelockAddress, mockTimelockABI, sepoliaProvider);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ async function main() {
const mockTimelockAddress = "0x471B3f60f08C50dd0eCba1bCd113B66FCC02b63d";
const mockTimelockJSON = "artifacts/contracts/bridges/test/MockTimelock.sol/MockTimelock.json";
contractFromJSON = fs.readFileSync(mockTimelockJSON, "utf8");
parsedFile = JSON.parse(contractFromJSON);
const parsedFile = JSON.parse(contractFromJSON);
const mockTimelockABI = parsedFile["abi"];
const mockTimelock = new ethers.Contract(mockTimelockAddress, mockTimelockABI, sepoliaProvider);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ async function main() {
const mockTimelockAddress = "0x471B3f60f08C50dd0eCba1bCd113B66FCC02b63d";
const mockTimelockJSON = "artifacts/contracts/bridges/test/MockTimelock.sol/MockTimelock.json";
contractFromJSON = fs.readFileSync(mockTimelockJSON, "utf8");
parsedFile = JSON.parse(contractFromJSON);
const parsedFile = JSON.parse(contractFromJSON);
const mockTimelockABI = parsedFile["abi"];
const mockTimelock = new ethers.Contract(mockTimelockAddress, mockTimelockABI, sepoliaProvider);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*global process*/
/*global process Buffer*/

const { ethers } = require("ethers");

Expand All @@ -22,7 +22,7 @@ async function main() {
const mockTimelockAddress = "0x471B3f60f08C50dd0eCba1bCd113B66FCC02b63d";
const mockTimelockJSON = "artifacts/contracts/bridges/test/MockTimelock.sol/MockTimelock.json";
contractFromJSON = fs.readFileSync(mockTimelockJSON, "utf8");
parsedFile = JSON.parse(contractFromJSON);
const parsedFile = JSON.parse(contractFromJSON);
const mockTimelockABI = parsedFile["abi"];
const mockTimelock = new ethers.Contract(mockTimelockAddress, mockTimelockABI, sepoliaProvider);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ async function main() {
const mockTimelockAddress = "0x471B3f60f08C50dd0eCba1bCd113B66FCC02b63d";
const mockTimelockJSON = "artifacts/contracts/bridges/test/MockTimelock.sol/MockTimelock.json";
contractFromJSON = fs.readFileSync(mockTimelockJSON, "utf8");
parsedFile = JSON.parse(contractFromJSON);
const parsedFile = JSON.parse(contractFromJSON);
const mockTimelockABI = parsedFile["abi"];
const mockTimelock = new ethers.Contract(mockTimelockAddress, mockTimelockABI, sepoliaProvider);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ async function main() {
const mockTimelockAddress = "0x471B3f60f08C50dd0eCba1bCd113B66FCC02b63d";
const mockTimelockJSON = "artifacts/contracts/bridges/test/MockTimelock.sol/MockTimelock.json";
contractFromJSON = fs.readFileSync(mockTimelockJSON, "utf8");
parsedFile = JSON.parse(contractFromJSON);
const parsedFile = JSON.parse(contractFromJSON);
const mockTimelockABI = parsedFile["abi"];
const mockTimelock = new ethers.Contract(mockTimelockAddress, mockTimelockABI, sepoliaProvider);

Expand Down
2 changes: 1 addition & 1 deletion test/veOLAS.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ describe("Voting Escrow OLAS", function () {
await olas.approve(ve.address, oneOLASBalance);
const lockDuration = oneWeek;

ve.createLock(oneOLASBalance, lockDuration);
await ve.createLock(oneOLASBalance, lockDuration);
await expect(
ve.createLock(oneOLASBalance, lockDuration)
).to.be.revertedWithCustomError(ve, "LockedValueNotZero");
Expand Down
Loading