Summary
Timeline: From 2025-04-28 To 2025-04-30
Total Issues: 25 (0 resolved)
Critical Severity Issues: 1 (0 resolved)
High Severity Issues: 1 (0 resolved)
Medium Severity Issues: 3 (0 resolved)
Low Severity Issues: 7 (0 resolved)
Notes & Additional Information: 13 (0 resolved)
Scope
This report evaluates the audit readiness of the in-scope contracts, highlighting concerns with the current design and providing recommendations for improvement.
Please note that the issues identified in this assessment are not exhaustive. To minimize delays arising from code revisions, only the most readily apparent and critical issues were included.
The OpenZeppelin team was asked to perform an assessment of the audit readiness of the camconrad/compensator repository at commit a436522.
In scope were the following files:
contracts
├── Compensator.sol
└── CompensatorFactory.sol
Overview
The Compensator
contract allows COMP token holders to delegate their voting power in exchange for rewards, encouraging increased participation in proposal votes. The contract is ERC-20-compliant and utilizes user balances and total supply for internal bookkeeping, with disabled transfer
and transferFrom
functionality. Delegates fund rewards for delegators by supplying COMP tokens into the Compensator
contract and setting a reward rate to distribute COMP rewards based on the proportional amount of tokens deposited by each delegator. A factory contract is utilized to deploy a unique Compensator
contract for each deployer, which delegates all voting power of its holdings to the delegate.
To prevent any single delegate from accumulating excessive voting power, a delegation cap of 5% of the total COMP supply is enforced. In addition, delegators can incentivize delegates to vote for or against specific proposals by staking their COMP tokens. After the voting period ends, delegators who staked for the winning option will pass their stake to the delegate, while those who staked for the losing option will have their stake returned.
Critical Severity
_updateRewardsIndex
Has Faulty Logic
The _updateRewardsIndex
function is the core function responsible for updating the reward index, which serves as the basis for calculating each user’s rewards. However, the mechanism that it uses to update availableRewards
and totalPendingRewards
is incorrect. The contract assumes the difference between availableRewards
and totalPendingRewards
to be equal to the available rewards inside the contract. Yet, when updating these values, the shared rewards are both subtracted from availableRewards
and added to totalPendingRewards
, which causes the result of availableRewards - totalPendingRewards
to decrease by twice the intended amount. This causes the internal bookkeeping of the protocol to be incorrect.
Consider fixing the logic of the _updateRewardsIndex
function and tracking the values correctly.
High Severity
Staking Mechanism Relies Solely on Delegate Without Independent Verification
The staking mechanism allows users to incentivize a delegate to vote for or against a certain proposal. The mechanism consists of three different stages:
- Users call
stakeForProposal
to lock in some COMP to incentivize a delegate with high voting power to vote in line with the users’ wishes. - The delegate calls
distributeStakes
to announce the proposal’s outcome and claims the staked amount that is in line with the result of the proposal execution. - Users who staked some amount on the other side of the proposal execution results can reclaim their locked stakes.
While the staking mechanism can be useful for incentivizing the delegate, it is too reliant on the delegate’s honest behavior. In a better mechanism, the delegate would have the power to take advantage of any of the following scenarios:
- The
distributeStakes
function does not check to see if the delegate actually provided the correct results to the proposal execution as the inputwinningSupport
is completely reliant on the delegate’s input. - The smart contract does not check to see whether the delegate actually voted for or against the staked proposal. Instead, it sends the staked amount to the delegate solely based on the result of the proposal execution.
- In case the delegate never calls
distributeStakes
after a proposal is executed, none of the users will be able to reclaim their staked amounts by callingreclaimLosingStake
.
Consider redesigning the staking mechanism in a manner that is less dependent on the delegate’s actions. One option could be to have the Compensator
contract verify the results of the proposal execution using the Compound Governor via the state function.
Medium Severity
Compensator
Does Not Account for Rewards When availableRewards
Is Less Than totalPendingRewards
The Compensator contract distributes rewards among all users based on the totalSupply
, ensuring that each token held by users earns an equal amount. However, when availableRewards
is less than totalPendingRewards
, no rewards are distributed among the delegators. This means that even if the contract is later funded by the delegate, it does not account for the users’ rewards earned from the earlier period.
Consider fixing the internal bookkeeping to ensure that rewards are correctly tracked and distributed to each user.
Compensator Rewards Can be Exploited by Withdrawing Before Voting Starts
The Compensator
contract allows users to opt-in via delegatorDeposit
or withdraw their COMP tokens via delegatorWithdraw
at any time, rewarding them based on the time their COMP tokens remained locked in the contract. However, since (1) there is no lock time for the funds in the contract and delegators have full control over their delegated funds, and because (2) Compound governance only considers the number of votes held by an account at the time of vote start, malicious users can withdraw their COMP tokens just before a proposal’s startBlock
, thereby retaining voting power and still receiving rewards from the Compensator contract.
Consider mitigating this issue by ensuring that users cannot avoid participating in certain proposals with their voting power. One potential solution could be to lock users’ funds for a certain amount of time.
Incomplete and Failing Test Coverage
The current test suite has multiple failing cases and does not consistently run as expected. In addition, the tests are not comprehensive and fail to cover critical edge cases and core contract behaviors. This affects confidence in the system’s reliability and makes it difficult to ensure correctness in various scenarios.
Consider fixing the failing tests and expanding the test coverage to include a wider range of potential edge cases.
Low Severity
rewardsUntil
Output Is Incorrect When Reward Rate Is Zero
The rewardsUntil
function returns the timestamp until which rewards will be distributed. However, if rewardRate
is zero, it returns the current block.timestamp
instead of the lastRewarded
timestamp, which may lead to confusing results.
To ensure accurate behavior, consider updating rewardsUntil
to return lastRewarded
when rewardRate
is zero.
getCompensators
Should Implement Pagination
The getCompensators function returns an array of all Compensator contract addresses. However, as the number of Compensators grows indefinitely, retrieving the full list may eventually become infeasible.
To ensure scalability, consider implementing pagination in getCompensators
to allow for fetching a subset of addresses at a time.
Floating Pragma
Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.
Throughout the codebase, multiple instances of floating pragma directives were identified:
Compensator.sol
has thesolidity ^0.8.21
floating pragma directive.CompensatorFactory.sol
has thesolidity ^0.8.21
floating pragma directive.
Consider using fixed pragma directives.
Incomplete Docstrings
Throughout the codebase, multiple instances of incomplete docstrings were identified:
- In
Compensator.sol
, theDelegateDeposit
event. For example, thedelegate
andamount
parameters are not documented. - In
Compensator.sol
, theDelegateWithdraw
event. For example, thedelegate
andamount
parameters are not documented. - In
Compensator.sol
, theRewardRateUpdate
event. For example, thedelegate
andnewRate
parameters are not documented. - In
Compensator.sol
, theDelegatorDeposit
event. For example, thedelegator
andamount
parameters are not documented. - In
Compensator.sol
, theDelegatorWithdraw
event. For example, thedelegator
andamount
parameters are not documented. - In
Compensator.sol
, theClaimRewards
event. For example, thedelegator
andamount
parameters are not documented. - In
Compensator.sol
, theProposalStaked
event. For example, thestaker
,proposalId
,support
, andamount
parameters are not documented. - In
Compensator.sol
, theProposalStakeDistributed
event. For example, theproposalId
andwinningSupport
parameters are not documented. - In
Compensator.sol
, theLosingStakeReclaimed
event. For example, thedelegator
,proposalId
, andamount
parameters are not documented - In
CompensatorFactory.sol
, theCompensatorCreated
event. For example, thedelegatee
andcompensator
parameters are not documented.
Consider thoroughly documenting all functions/events (and their parameters or return values) that are part of a contract’s public API. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Redundant State Updates and Misleading Events
When a setter function updates a state variable without first checking whether the new value differs from the current one, it will overwrite the storage with the same data. This operation still consumes gas and, more importantly, emits an event signaling state change—even though no actual change occurred.
The setRewardRate
function sets the rewardRate
variable and emits an event without checking if the value has changed.
Consider adding a check that reverts the transaction if the value being set is identical to the existing value.
Improper Use of Local @openzeppelin/contracts
Folder
The project contains a folder named @openzeppelin/contracts
which should not exist as a local directory. This setup can lead to confusion, versioning issues, and potential misuse of core library files.
Consider removing the folder and relying solely on package-managed dependencies to ensure proper functioning and compliance with best practices.
Unnecessary Use of Initializable
in Non-Upgradeable Contract
The Compensator contract inherits from Initializable
. This inheritance pattern is intended for upgradeable contracts. However, since the contract is only deployed via a factory and the initialize
function is called immediately after deployment, this pattern is unnecessary and could be replaced with a constructor.
Consider removing the initialize
function and using a constructor instead. Alternatively, to benefit from the upgradeable pattern and reduce gas costs, consider having the factory deploy minimal proxies pointing to a single implementation contract.
Notes & Additional Information
Functions Updating State Without Event Emissions
Throughout the codebase, multiple instances of functions updating the state without an event emission were identified:
- The
initialize
function inCompensator.sol
- The
_updateUserRewards
function inCompensator.sol
- The
_updateRewardsIndex
function inCompensator.sol
Consider emitting events whenever there are state changes to improve the clarity of the codebase and make it less error-prone.
Redundant Getter Function
When state variables use public
visibility in a contract, a getter method is automatically generated by the compiler.
Within the CompensatorFactory
contract in CompensatorFactory.sol
, the getCompensator
function is redundant because the delegateeToCompensator
state variable, being a public
variable, already has a getter. In addition, the getCompensator
function is redundant because the delegateeToCompensator
state variable, being a public
variable, already has a getter.
To improve the overall clarity, intent, and readability of the codebase, consider removing the redundant getter functions.
Lack of Security Contact
Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.
Throughout the codebase, multiple instances of contracts not having a security contact were identified:
Consider adding a NatSpec comment containing a security contact above each contract definition. Using the @custom:security-contact
convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Missing Named Parameters in Mappings
Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?)
. This updated syntax provides a more transparent representation of a mapping’s purpose.
Throughout the codebase, multiple instances of mappings without named parameters were identified:
- The
startRewardIndex
state variable in theCompensator
contract. - The
unclaimedRewards
state variable in theCompensator
contract. - The
proposalOutcomes
state variable in theCompensator
contract. - The
proposalStakes
state variable in theCompensator
contract. - The
totalStakesFor
state variable in theCompensator
contract. - The
totalStakesAgainst
state variable in theCompensator
contract. - The
delegateeToCompensator
state variable in theCompensatorFactory
contract.
Consider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.
Non-Explicit Imports
The use of non-explicit imports in the codebase can affect code clarity and may create naming conflicts between locally defined and imported variables. This is particularly relevant when multiple contracts exist within the same Solidity file or when inheritance chains are long.
Throughout the codebase, multiple instances of non-explicit imports were identified:
- The import “./IComp.sol”; import in
Compensator.sol
- The import “./IGovernor.sol”; import in
Compensator.sol
- The import “@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol”; import in
Compensator.sol
- The import “@openzeppelin/contracts/token/ERC20/ERC20.sol”; import in
Compensator.sol
- The import “@openzeppelin/contracts/proxy/utils/Initializable.sol”; import in
Compensator.sol
- The import “./Compensator.sol”; import in
CompensatorFactory.sol
Following the principle that clearer code is better code, consider using the named import syntax (import {A, B, C} from "X"
) to explicitly declare which contracts are being imported.
Lack of Indexed Event Parameter
Within Compensator.sol
, the ProposalStakeDistributed
event does not have indexed parameters.
To improve the ability of off-chain services to search and filter for specific events, consider indexing event parameters.
Inconsistent Order Within a Contract
The Compensator
contract deviates from the Solidity Style Guide due to having inconsistent ordering of functions.
To improve the project’s overall legibility, consider standardizing ordering throughout the codebase as recommended by the Solidity Style Guide (Order of Functions).
Hardcoded Addresses
In Compensator.sol
, multiple instances of addresses hardcoded address
values were identified:
- The
0xc00e94Cb662C3520282E6f5717214004A7f26888
value - The
0x309a862bbC1A00e45506cB8A802D1ff10004c8C0
value
Consider declaring hardcoded addresses as immutable
variables and initializing them through constructor arguments. This allows code to remain the same across deployments on different networks and mitigates situations where contracts need to be redeployed due to having incorrect hardcoded addresses.
Magic Numbers
Within Compensator.sol
, multiple instances of literal values with unexplained meanings were identified.
Consider defining and using constant
variables instead of using literals to improve the readability of the codebase.
Function Visibility Overly Permissive
Throughout the codebase, multiple instances of functions with unnecessarily permissive visibility were identified:
- The
initialize
function inCompensator.sol
withpublic
visibility could be limited toexternal
. - The
_updateUserRewards
function inCompensator.sol
withinternal
visibility could be limited toprivate
. - The
_updateRewardsIndex
function inCompensator.sol
withinternal
visibility could be limited toprivate
. - The
_getCurrentRewardsIndex
function inCompensator.sol
withinternal
visibility could be limited toprivate
. - The
getCompensators
function inCompensatorFactory.sol
withpublic
visibility could be limited toexternal
.
To better convey the intended use of functions and to potentially realize some additional gas savings, consider changing a function’s visibility to be only as permissive as required.
Multiple Optimizable State Reads
In Compensator.sol
, multiple opportunities for optimizing storage reads were identified:
- The
rewardRate
storage read - The
availableRewards
storage read - The
totalPendingRewards
storage read - The
delegate
storage read - The
delegate
storage read - The
availableRewards
storage read - The
totalPendingRewards
storage read - The
totalPendingRewards
storage read - The
availableRewards
storage read - The
rewardIndex
storage read
Consider reducing SLOAD operations that consume unnecessary amounts of gas by caching the values in a memory
variable.
Constants Not Using UPPER_CASE
Format
In Compensator.sol
, multiple instances of constants not being declared using the UPPER_CASE
format were identified.
According to the Solidity Style Guide, constants should be named with all capital letters with underscores separating words. For better readability, consider following this convention.
Use calldata
Instead of memory
When dealing with the parameters of external
functions, it is more gas-efficient to read their arguments directly from calldata
instead of storing them to memory
. calldata
is a read-only region of memory that contains the arguments of incoming external
function calls. This makes using calldata
as the data location for such parameters cheaper and more efficient compared to memory
. Thus, using calldata
in such situations will generally save gas and improve the performance of a smart contract.
Within CompensatorFactory.sol
, the delegateeName
parameter should use calldata
instead of memory
.
Consider using calldata
as the data location for the parameters of external
functions to optimize gas usage.
Conclusion
The codebase in its current state is not yet ready for a comprehensive security audit due to the technical and design issues identified in this assessment, as well as the failing test suite. To improve its audit readiness, the system should integrate Compound Governor’s trust assumptions regarding voting delegation into the deposit/withdrawal and staking logic while enabling permissionless querying of proposal states through the designated contract. Additionally, the test suite must be revised to ensure that all tests pass and to achieve sufficient coverage for sensitive functions and edge cases.