Codebase from three different repositories.
- Review Summary
- Code Evaluation Matrix
- Findings Explanation
- Critical Findings
- High Findings
- Medium Findings
- 1. Medium - Potential denial of service vulnerability in bridger contracts
- 2. Medium - Child gauge vulnerability: uncontrolled reward addition by managers
- 3. Medium - unkillGauge should checkpoint funds distribution to prevent distributing for the time it was killed
- 4. Medium - Potential user exploitation of boost adjustment mechanism
- Low Findings
- 1. Low - Rewards can be locked in Gauges
- 2. Low - Add missing input validation on constructor/initializer/setters
- 3. Low - Potential user manipulation of Uniswap deposit to claim rewards within range
- Gas Findings
- Informational Findings
- 1. Informational - Potential transaction order discrepancy and impact on total supply calculation in sidechain
- 2. Informational - Optimizing bytecode size by removing unused constant variable
- 3. Informational - Missing events for critical operations
- 4. Informational - Leveraging new syntax in Vyper 0.3.4 for default return value
- Final remarks
Timeless gauges are emulating the mechanics of curve multi-chain gauges from Curve, with a handful of modifications. Small adjustments have been made to the gauge contracts to facilitate updates to the boost, as well as eliminating the reliance on anyCall. Additionally, they’ve independently created their own ve balance cross-chain contracts.
The contracts of three timeless repositories were reviewed over 21 days. The code review was performed by 2 auditors between May 22 and June 9, 2023. The repository was under active development during the review, but the review was limited to the latest commit at the start of the review. This was commit 5236bc3d6bcd331d1ebf1bc8e80fe61163f937c2 for the ve-beacon repo, commit fb5f03691972ad3d3edce534f236a47c586eecaf for the universal-bridge-lib repo, and commit 41344e3197ce7b007f390c15360234a6e159ad65 for the gauge-foundry repo.
The scope of the review consisted of the following contracts in three different repositories:
├── src │ ├── VeBeacon.sol │ ├── VeRecipient.sol │ ├── base │ │ ├── BoringOwnable.sol │ │ └── Structs.sol │ ├── interfaces │ │ └── IVotingEscrow.sol │ └── recipients │ ├── VeRecipientAMB.sol │ ├── VeRecipientArbitrum.sol │ ├── VeRecipientOptimism.sol │ └── VeRecipientPolygon.sol
├── src │ ├── UniversalBridgeLib.sol │ └── bridges │ ├── ArbitraryMessageBridge.sol │ ├── ArbitrumBridge.sol │ ├── OptimismBridge.sol │ └── PolygonBridge.sol
├── vyper_contracts │ ├── ChildGauge.vy │ ├── ChildGaugeFactory.vy │ ├── GaugeController.vy │ ├── RootGauge.vy │ ├── RootGaugeFactory.vy │ └── bridgers │ ├── ArbitrumBridger.vy │ ├── ArbitrumRefund.vy │ ├── OmniBridger.vy │ ├── OptimismBridger.vy │ └── PolygonBridger.vy
This review is a code review to identify potential vulnerabilities in the code. The reviewers did not investigate security practices or operational security and assumed that privileged accounts could be trusted. The reviewers did not evaluate the security of the code relative to a standard or specification. The review may not have identified all potential attack vectors or areas of vulnerability.
yAudit and the auditors make no warranties regarding the security of the code and do not warrant that the code is free from defects. yAudit and the auditors do not represent nor imply to third parties that the code has been audited nor that the code is free from defects. By deploying or using the code, Timeless Finance and users of the contracts agree to use the code at their own risk.
|Access Control||Good||Properly access controlled. One case of DoS should be fixed.|
|Mathematics||Good||The mathematical operations are secure with Solidity 0.8 and Vyper. The Ve Beacon contracts adopt the decay mechanisms found in VotingEscrow.vy.|
|Complexity||Good||Contracts are well designed.|
|Libraries||Average||The utilization of libraries has been executed effectively.|
|Decentralization||Good||The protocol can function without the need for the admin.|
|Code stability||Good||All key features have been successfully implemented, and the code has demonstrated stability throughout the review process.|
|Documentation||Average||We didn’t have access to documentation during the audit.|
|Monitoring||Average||Some functions are missing event emission.|
|Testing and verification||Low||Conducting tests on fund flows using suitable bridges is recommended, even though we acknowledge the complexity and difficulty involved in this task.|
Findings are broken down into sections by their respective impact:
- Critical, High, Medium, Low impact
- These are findings that range from attacks that may cause loss of funds, impact control/ownership of the contracts, or cause any unintended consequences/actions that are outside the scope of the requirements
- Gas savings
- Findings that can improve the gas efficiency of the contracts
- Findings including recommendations and best practices
Using self-destruct, an attacker can send ETH to the contract, making it unusable.
The following code reverts when ETH is left on the contract.
if (address(this).balance != 0) revert VeBeacon__LeftoverEth();
This ensures users don’t overpay for the Arbitrum bridge, but it has two issues:
- An attacker will be able to DOS the contract by sending some ETH dust that will remain, causing the check to revert.
- A user might get his transaction reverted because the Arbitrum fee might change between the time the transaction is broadcasted and the time it gets executed.
High. The contract can become unusable.
Refund the unused ETH if the cost of the gas to send back the ETH is lower than the amount to be sent back.
This can be done using the
block.basefee and the standard gas cost of a transfer of 21k.
Fixed in https://github.com/timeless-fi/ve-beacon/pull/1/commits/a47a2a6a1df43dc45fb524e5e9692687a5feb81f
The Bridger contracts use the
ERC20(_token).approve() function calls. Some tokens like USDT do not return a value for these calls, resulting in the assert statement constantly failing. This could cause a Denial of Service (DoS) for these tokens.
When a user tries to bridge tokens like USDT that doesn’t return a boolean value on
approve() function calls, the operation will fail since the Bridger contracts use the assert statement to validate the function call’s success. In Vyper, the assert function throws an exception if the condition is unmet and reverts all changes.
Medium. Potential incompatibility or denial of service for certain tokens.
- A recommended fix is to make a low-level raw_call to the ERC20 token contracts. This will help manage the return value properly, as it gives you more control over the execution of the contract call. This approach is particularly useful when working with contracts that might not fully comply with the standard, like USDT. Here’s an example of how it can be implemented in Vyper:
response: Bytes = raw_call( _token, _abi_encode( msg.sender, self, _amount, method_id=method_id("transferFrom(address,address,uint256)") ), max_outsize=32, ) if len(response) != 0: assert convert(response, bool)
This is just an example to show the approach. You need to adapt this example to fit in the actual contract code.
- Another solution could be to use the default_return_value option in the transferFrom and approve function calls. Here’s how it could be implemented in Vyper:
ERC20(_token).transferFrom(msg.sender, self, _amount, default_return_value=True)
However, be cautious when using the default_return_value option, as it could potentially mask other issues. If the transferFrom function does not return a value due to a problem unrelated to tokens like USDT, this code will still consider the function call successful. Use this option carefully and only if it is suitable for your specific context.
Fixed in https://github.com/timeless-fi/gauge-foundry/pull/1/commits/b982da598c2cf8da51e6c473f701a84400b29895
With the current design, any user can create a child gauge and become the manager of that child gauge. While the team can later change the manager using
set_manager(_manager: address), before the update occurs, a manager can add multiple rewards by calling
add_reward(_reward_token: address, _distributor: address) and reach the limit of rewards defined as
MAX_REWARDS. This can prevent the distribution of actual rewards.
Refer to ChildGauge.vy#L579 for the relevant code snippet.
Medium. Lack of access control for gauge creation and initial ownership can be problematic.
Consider one of the following options:
- Implement a remove reward function.
- Restrict access to
deploy_gaugeto trusted entities.
- Set the default manager as the factory owner.
Fixed in https://github.com/timeless-fi/gauge-foundry/pull/1/commits/a998eaa6372287f7224bdc54302851900aa70d1d
3. Medium - unkillGauge should checkpoint funds distribution to prevent distributing for the time it was killed
When a gauge is restored using the unkillGauge() method, users of that gauge can claim rewards retroactively, back to the point when the gauge was ‘killed’.
The function unkillGauge() merely changes the gauge state flag to active. User reward claims will trigger a checkpoint that will distribute rewards for the period up until the last checkpoint, which was set when the gauge was ‘killed’. Consequently, this distributes rewards corresponding to the duration in which the pool was inactive.
Medium. Funds could be distributed for the entire duration the pool was in the ‘killed’ state.
We suggest updating the
period_timestamp state variables when the
unkillGauge method is invoked.
Fixed in https://github.com/timeless-fi/gauge-foundry/pull/1/commits/f2fe0a1b49b01824eff5894746976202648c1aff
In the ChildGauge module, the team implemented a method called set_tokenless_production(). This function alters the boost value for a gauge. When the boost is decreased, any user not actively interacting with the gauge will continue to benefit from the previous boost level until their next interaction with the gauge.
The function at ChildGauge.vy#L699 sets a new boost value. While a ‘kick’ function exists, it cannot be utilized to update a user’s effective balance. The kick() method can only be used when the user has updated a lock.
Medium. Users can exploit these mechanics to earn more rewards.
We recommend modifying the ‘kick’ function to allow for the update of the effective balance if it has diverged from the original effective balance by a predetermined percentage.
Fixed in https://github.com/timeless-fi/gauge-foundry/pull/1/commits/a03f79ba84895fcfc4fc295e7f3cbb48386fce85
There’s a risk that rewards may become inaccessible when depositing smaller quantities into the gauge.
childGuage.vy function deposit_reward_token allows anyone to create, and rewards distribution is done using the reward rate. Still, if the reward
_amount (in WEI) less than
1 WEEK (or 604800 Wei), the transferred rewards will be locked into the contract.
self.reward_data[_reward_token].rate = _amount / WEEK
_amount < 604800 then
self.reward_data[_reward_token].rate = 0.
Use scaling factor for reward rate with scale >= 1e6.
self.reward_data[_reward_token].rate = (_amount * REWARD_SCALE) / WEEK
Fixed in https://github.com/timeless-fi/gauge-foundry/pull/1/commits/88dcc705686bf0c6ff134821428250fbe6bd789b
Validating inputs is critical prior to establishing important variables.
Certain functions are missing necessary input validation.
- In childGaugeFactory.vy:
- In childGuage.vy:
Low. Proper input validation can help revert when important state variables are about to be set with wrong values.
Consider implementing all the checks suggested above.
Acknowledged, won’t fix.
A Gauge can be configured for automated distribution of rewards, which triggers when a Uniswap v3 deposit is within the swap range. However, a user could manipulate this system to claim rewards by sandwiching an out-of-range deposit, thereby garnering rewards from the gauge.
For more information, refer to ChildGauge.vy#L744.
Low. Funds could be distributed inappropriately.
As we don’t have access to
UNISWAP_POOR_ORACLE, it could be used as a safeguard against price manipulation within the same block or transaction.
The Uniswap Price-out-of-range oracle is secure against sandwich attacks, so such manipulation is not feasible. See https://github.com/timeless-fi/uniswap-poor-oracle
_broadcastVeBalance() function currently checks for the votingEscrow global epoch after verifying the user epoch. However, since creating a lock always increases the global epoch, the assert statement is unnecessary.
The following code snippet is not necessary:
if (epoch == 0) revert VeBeacon__EpochIsZero();
Consider removing the assert statement as it is unnecessary.
Fixed in https://github.com/timeless-fi/ve-beacon/pull/1/commits/6395e5a5f7c973bc99bc98cdd4005d3cec6bb1e3
Multiple functions are made to set the same value, they could be grouped together.
Multiple setters are currently used to modify the
gauge_state value. To optimize gas usage, it is recommended to consolidate these setters into a single function.
To reduce code size and enhance efficiency, consider utilizing a Vyper enum instead of numbers and implement a single setter function.
Acknowledged, won’t fix.
1. Informational - Potential transaction order discrepancy and impact on total supply calculation in sidechain
In an extreme case where the transaction order is not respected on the sidechain, the total supply might not be correct for a short period of time.
There is no warranty on transaction ordering from L1 to L2. A transaction can be mined on L1, fail on L2, and then be retried later, resulting in a different order.
Here is an example:
- User A creates a lock that will last for 4 years.
- User A broadcasts their lock to L2.
- User B creates a lock that will last two weeks.
- User B broadcasts their lock to L2.
If the broadcast from User B is processed by L2 before the one from User A, then the slope changes corresponding to the end of the lock from User B will be removed from the corresponding variable when User A’s transaction is processed on L2.
Monitor this potential edge case. The impact of such a small lock is negligible, as the user’s balance will go to zero regardless. The total supply will be fixed once a new update is sent to L2.
Having an unused constant variable not only increases the size of the bytecode but can also confuse developers with unnecessary variables.
There is an unused constant variable in the contract childGaugeFactory.vy that should be removed.
Informational. Removing the unused variable will result in a reduction in the deployed bytecode size.
Remove the unused variable from the contract.
Fixed in https://github.com/timeless-fi/gauge-foundry/pull/1/commits/76baf36dbddb1b496cb2e9e0034c6b2ee59a1730
Several critical operations do not trigger events. As a result, it is difficult to check the behavior of the contracts.
Ideally, the following critical operations should trigger events.
childGuage, following function lacks event emission.
rootGuage, following functions lacks event emission.
Informational. Without events, users and blockchain-monitoring systems cannot easily detect suspicious behavior.
Add events for all critical operations. Events aid in contract monitoring and the detection of suspicious behavior.
Fixed in https://github.com/timeless-fi/gauge-foundry/pull/1/commits/25225cfce5af64cfce655dd63089771d90026d20 and https://github.com/timeless-fi/ve-beacon/pull/1/commits/6b558e15076589eec70f1612d545253931758e4a
The latest version, vyper 0.3.4, introduces a new syntax that allows the addition of a default_return_value to a call.
The existing codebase uses low-level raw_calls for safe calls. This can be simplified using the new syntax.
Consider employing the new syntax:
response: Bytes = raw_call( token, _abi_encode( receiver, total_claimable, method_id=method_id("transfer(address,uint256)") ), max_outsize=32, ) if len(response) != 0: assert convert(response, bool)
This can be replaced by:
assert ERC20(token).transfer(receiver, total_claimable, default_return_value=True)
Fixed in https://github.com/timeless-fi/gauge-foundry/pull/1/commits/96780f1c28043f8a9f55595f02ef5019f807ae1b
In summary, the code review of the Timeless Finance contracts revealed findings of varying impact levels. While the contracts exhibit strong design and implementation, featuring proper access control, secure mathematical operations, and decentralization capabilities, there are specific areas that need to be addressed to enhance functionality and security.