Bancor Compounding Rewards Audit
August 4, 2022
This security assessment was prepared by OpenZeppelin, protecting the open economy.
Table of Contents
- Table of Contents
- Rewards system overview
- Security model and trust assumptions
- High Severity
- Medium Severity
- Low Severity
- Notes & Additional Information
The client asked us to review the smart contracts related to the auto-compounding rewards module, including the mathematical dependencies and how the pool contracts are used by the rewards module.
- From 2022-06-06
- To 2022-06-17
- Total Issues
- 12 (2 resolved, 2 partially resolved)
- Critical Severity Issues
- 0 (0 resolved)
- High Severity Issues
- 1 (1 resolved)
- Medium Severity Issues
- 3 (0 resolved)
- Low Severity Issues
- 5 (1 partially resolved)
- Notes & Additional Information
- 3 (1 resolved, 1 partially resolved)
We audited commit b2224595afab14827ee065cb65a1444d61689fb3 of the bancorprotocol/contracts-v3 repository.
In scope were the following contracts:
contracts ├── pools │ ├── BNTPool.sol │ | └── poolTokenAmountToBurn() │ └── PoolCollection.sol │ └── poolTokenAmountToBurn() ├── rewards │ ├── AutoCompoundingRewards.sol │ ├── RewardsMath.sol │ └── interfaces │ └── IAutoCompoundingRewards.sol └── utility ├── Constants.sol ├── Fraction.sol ├── FractionLibrary.sol └── MathEx.sol
All contracts not explicitly mentioned above are out of scope for this audit. The
pools contracts were only audited to the extent that
Rewards system overview
Token projects, including Bancor’s
BNT, can create a rewards schedule by providing liquidity to a pool and then burning the pool tokens it has received at a predetermined rate. Thus, the rewards budget is used as liquidity, and the ownership of the staked tokens is distributed to other stakers over time.
To initialize a rewards program,
TKN tokens are added to the pool, and
bnTKN pool tokens are issued as normal (or
bnBNT for the
BNT token). Then, the pool tokens are staked into a generic rewards contract, with customizable parameters, such as the number of tokens to distribute, the emission rate, and the emission profile (linear or exponential).
Interactions with the Vortex contract cause the pool tokens to be burned in accordance with the parameters set by the rewards contract; the Vortex trigger also triggers the rewards distribution. As pool tokens are burned, the token value they represent is effectively distributed to the remaining pool tokens. Thus, no contract interactions are required by stakers; so long as they hold the pool token, they are automatically participating in the rewards program.
The auto compounding system can be used in any instance where the reward token is the same as the staked token; it cannot be used to distribute
TKN, or to distribute
TKN to any pool other than itself. For use cases where the reward token is different than the staked token, a standard rewards contract is available. The standard rewards contract is out of scope for this audit.
AutoCompoundingRewards contract relies on the
ROLE_ADMIN role and
onlyAdmin modifier to protect the following functions:
setAutoProcessRewardsCount– Sets the number of programs to auto-process the rewards for each time
terminateProgram– Terminates a rewards program
enableProgram– Changes a rewards program status between enabled and disabled
createProgram– Creates a rewards program for a given pool
Security model and trust assumptions
The Bancor protocol is controlled by an active DAO.
In addition to updating the system via previously mentioned sensitive variables, the protocol is also arbitrarily upgradeable. The DAO has unrestricted control over the protocol, and we assume that the DAO is acting in the best interest of users and the system.
Here we present our findings.
Given the relatively small scope of this audit with respect to the size of the entire project, we assess the severity of findings based on our recommendations for best practices without consideration of whether there are mitigations in other out-of-scope parts of the codebase. For libraries, in particular, we consider how these may be improved upon for safe reuse in potential future iterations or new applications.
reducedFraction does not reduce in the mathematical sense
MathEx library, the function
reducedFraction does not reduce the Fraction, instead it truncates the numerator and denominator in a way that ensures it is lower than a given maximum. So, rather than using the greatest common denominator between the numerator and denominator to reduce the fraction, this
reducedFraction function merely prunes the numerator and denominator to ensure that they are less than the provided max, and, in so doing, loses precision. In some instances the deviation between the fraction and the result of
reducedFraction can be over 10%.
Consider either revising the function to reduce the loss of precision or renaming the function to
truncateFraction and providing more inline documentation that makes the loss of precision clear.
Update: Fixed in commit
2cb941a636dd9b0c210dd52ee90907eeb02eea6a and commit
weightedAverage calculation might overflow
MathEx library the function
weightedAverage performs a multiplication of up to three
uint256 input variables without any prior validation. As the resulting number can have up to
768 bits, it is very likely to lead to an overflow during
Currently, the function
weightedAverage is only used in the
_calcAverageRate function in the
PoolCollection contract. Further, the
_calcAverageRate function is currently used in a way that ensures a maximum size of
3 bits for the three operands passed to
While the current usage of
weightedAverage within the codebase appears to be safe from
uint256 overflows, consider adding input validation to
weightedAverage and including explicit preconditions in the function documentation to ensure that future versions of the codebase remain free from overflows.
Update: Acknowledged without code changes. In an effort to keep library functions lean, the client generally prefers panics over explicit error messages. Note that the function documentation continues to omit any description about preconditions.
calcExpDecayRewards is at risk of overflow reversions
RewardsMath contract the
calcExpDecayRewards function takes
halfLife values as input, puts them into a
Fraction struct, and then passes them along to the
MathEx.exp2 function. There is a comment in the inline documentation that
because the exponentiation function is limited to an input of up to (and excluding) 16 / ln 2, the input value to this function is limited by
timeElapsed / halfLife < 16 / ln 2
This is problematic because, for any exponential decay distribution program,
halfLife is fixed while
timeElapsed will continue to grow after the program is created. For any such program then, it is only a matter of time before
halfLife by more than the required ratio to overflow a call to
timeElapsed / halfLife exceeds this limit, then the call will simply revert in the
exp2 function with an
Overflow error. This will cause the entire transaction to revert. Since
calcExpDecayRewards is called by the
_tokenAmountToDistribute function, which is called by the
_processRewards function, no rewards for a program that reaches such a state will be possible.
In such a scenario where the
timeElapsed is too large, rather than making the call to
exp2 and then reverting, consider having the
calcExpDecayRewards function return
totalRewards without any exponentiation calculation. This will result in minimal error (i.e.,
1 * totalRewards instead of
0.9999998874648253 * totalRewards), but will prevent the case where a program’s rewards cannot be distributed because too much time elapses from the program’s start.
Update: Acknowledged without code changes. The client expressed that the function inputs are safe for timeframes considered practical. In the case of impractical timeframes the impact is acceptable.
Lack of validation
Throughout the codebase, there are places where a proper input/output validation is lacking. In particular:
FractionLibrarydo not check if they were provided an invalid fraction.
mulDivFfunctions in the
MathExlibrary do not check that
zin non-zero. In such cases they revert with a Panic, rather than a more helpful error message.
weightedAveragefunctions in the
MathExlibrary do not check that the input fractions are valid. The latter also does not ensure it is returning a valid fraction.
A lack of validation on user-controlled parameters may result in erroneous or failing transactions that are difficult to debug. This is especially true in the case of libraries that are capable of being reused across different codebases. To avoid this, consider adding input and output validation to address the concerns raised above.
Update: Acknowledged without code changes. The client expressed that the lack of reversion on invalid inputs is intentional. In some cases it is desirable behavior to turn divide by zero inputs into zero outputs. In other cases, error messages will be raised somewhere else along the call chain in practice.
Unbounded number of programs could result in errors
programs function loops over a potentially unbounded list of programs/pools.
pools function requires that all stored values associated with stored pools are first loaded into memory for the call to complete.
In either case, if too many programs are added to the protocol, these functions will run out of gas.
Although neither of these functions are used internally, should the number of programs grow too large, they could be problematic for external contracts that might rely on them.
Consider either adding a pagination mechanism to limit the number of stored values that must be loaded or adding some warning to the documentation about the need to keep the number of programs bounded to some safe level to avoid issues with these functions.
Update: Acknowledged. The client conveyed that simplicity of the external interface was an intentional design choice. If future on-chain usage becomes relevant the respective contract will be upgraded.
The internal logic of the
poolTokenAmountToBurn functions in the
PoolCollection contracts are essentially duplicates.
Duplicating code can lead to issues later in the development lifecycle and leaves the project more prone to the introduction of errors. Such errors can inadvertently be introduced when functionality changes are not replicated across all instances of code that should be identical.
Rather than duplicating code, consider having just one contract or library containing the duplicated code and using it whenever the duplicated functionality is required.
Update: Acknowledged. The flexibility of independent upgrades is prioritized over re-use in this case. Client’s response:
If the contracts are used for a while and the shared logic is kept in place without frequent changes, a shared logic component will be created, but for now flexibility is more important than code reusability, especially since these two contracts are very different conceptually.
uint32 times can overflow in the future
AutoCompoundingRewards contract, all program start, end, and elapsed times are encoded as
uint32 values. In the year 2106, this will be problematic in two ways.
- Near the time when unix timestamps will begin to overflow the
uint32type, it will become difficult to create flat programs ending after the unix timestamp overflow event. Specifically, it will not be possible to call
endTimethat overflows the
uint32type because it will appear to be less than the
startTime. This will be a temporary inconvenience, because once the current time overflows the
uint32type it will be possible to create new flat programs as normal again.
createExpDecayProgramdoes not expect an
endTimeinput. Instead, a
halfLifeinput parameter is specified. This will make it possible to call
createExpDecayProgrameven around the time that unix time will overflow the
uint32type. However, if a program is created before the overflow time, then after the overflow time, even if it should be a valid program, it would be treated as inactive and no rewards from the program could be processed. This, in turn, would prevent both
autoProcessRewardsfrom processing rewards for that program. This would be more problematic and tight packing of the stored programs could make an update to remedy the situation more difficult.
Consider either increasing the bits available to store and evaluating timestamps to something larger than
uint32 or adding a warning to the protocol documentation to upgrade the contract well before the overflow time will be reached.
Update: Acknowledged. Client’s response:
The contracts are expected to change and evolve and new versions implemented way before the timestamps can overflow (at 2106).
Lack of event emissions after updating state
The following functions do not emit relevant events despite executing state-altering actions:
- No event is emitted when the
_autoProcessRewardsCountvariable is first set during contract initialization. This is inconsistent with the fact that an event is emitted when the same value is updated via the
- There is no event emitted when
_autoProcessRewardsIndexis updated despite the fact that this value influences the behavior of the
In order to facilitate tracking and to notify off-chain clients following the contracts’ activity, consider emitting events whenever state-changing operations take place.
Update: Partially addressed in commit
5f223b2ff0f1f9fcdd92d89bc2e420c9d984cb5c. An event including the new value of
_autoProcessRewardCount is now emitted during contract construction. However, the client considers
_autoProcessRewardsIndex an non-public implementation detail which should not emit events upon alteration, despite its role in facilitating monitoring.
enableProgram is overloaded
AutoCompoundingRewards contract, the
enableProgram function is used to enable or disable a program for a specific pool, depending on the bool
status parameter. The function also emits a
ProgramEnabled event, even in the instance where the program is being disabled (although the event details contain the correct updated status).
In order to avoid unnecessary confusion and to increase the overall readability of the codebase, consider renaming the function and event to something such as
updateProgramStatus to better reflect the actual function operation.
Update: Acknowledged. Client’s response:
This was a design decision in order to prioritize ease of consumption (single event) and binary size over readability.
Notes & Additional Information
Confusing use of
assert statement in the
calcFlatRewards function of the
RewardsMath library checks a condition that is impossible only because of how it is used within the
_tokenAmountToDistribute function. However, given the
calcFlatRewards function definition and use of the
programDuration parameters, the purpose of the
assert statement is not apparent within the
To enforce input validation within the
calcFlatRewards function, consider changing the
assert statement to a
require, and providing an error message explaining the validation. Alternatively, consider moving the
assert statement into the
_tokenAmountToDistribute function to check that the invariant holds in that context.
Update: Remains unaddressed in commit
b2224595afab14827ee065cb65a1444d61689fb3. While the documentation of the function improved in commit
ce5d2e73b348fcc368157edd70a5e59d043d939d, the function still uses
assert to check a condition that is not an invariant.
AutoCompoundingRewards contract the
AccessDenied import is unused.
Consider removing unused import statements to simplify the codebase and increase overall readability.
Update: Fixed as of commit
107b3aa85bc306a0d94705a0bd816e57267bd9f7 in pull request #430.
Throughout the codebase there are numerous functions missing or lacking documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security, but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned and the events emitted.
Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Acknowledged. The comments will be added in future versions of the codebase.
1 high and 3 medium severity issues were found. Some changes were proposed to follow best practices and reduce the potential attack surface.