Anvil Protocol Audit

OpenZeppelin Security | October 11, 2024

 

Table of Contents

Summary

Type
DeFi
Timeline
From 2024-04-08
To 2024-05-03
Languages
Solidity
Total Issues
32 (32 resolved)
Critical Severity Issues
1 (1 resolved)
High Severity Issues
2 (2 resolved)
Medium Severity Issues
1 (1 resolved)
Low Severity Issues
9 (9 resolved)
Notes & Additional Information
19 (19 resolved)

Scope

We audited the AmperaFoundation/sol-contracts repository at commit c6e940c.

In scope were the following files:

 contracts
├── CollateralVault.sol
├── Pricing.sol
├── TimeBasedCollateralPool.sol
├── governance
│   ├── Anvil.sol
│   ├── AnvilERC20Votes.sol
│   ├── AnvilGovernorDelegate.sol
│   ├── AnvilGovernorDelegator.sol
│   ├── AnvilTimelock.sol
│   ├── AnvilVotes.sol
│   ├── Claim.sol
│   └── IClaimable.sol
└── interfaces
    ├── ICollateral.sol
    ├── ICollateralDepositTarget.sol
    ├── ICollateralPool.sol
    └── ITimeBasedCollateralPool.sol

Update: The Acronym Foundation team has moved the Anvil contracts to a new repository which can be found at https://github.com/AcronymFoundation/anvil-contracts. We have validated that the in-scope contracts as of commit c6e940c from the AmperaFoundation/sol-contracts repository match exactly with the in-scope contracts as of commit be6bd02 from the AcronymFoundation/anvil-contracts repository. Further, we can confirm that both repositories use the same version of the OpenZeppelin contracts and contracts-upgradeable libraries. It should be noted that the AcronymFoundation/anvil-contracts repository uses solc 0.8.25 whereas the AmperaFoundation/sol-contracts codebase used 0.8.20, however this difference is in-line with the change that was reviewed in issue L-09.

System Overview

The Anvil protocol is an Ethereum-based collateral protocol providing transparent and efficient collateral management to the smart contracts within its ecosystem.

CollateralVault and TimeBasedCollateralPool

The CollateralVault contract is the backbone of the system and acts as the central point of collateral management, interacting with the approved collateralizable contracts of the ecosystem. The users can deposit collateral in the CollateralVault contract and approve a specific allowance amount to each collateralizable contract of their preference. Then, any collateral management action needed for their account in the collateralizable contract is settled transparently in cooperation with the CollateralVault contract.

More specifically, users interact with CollateralVault to deposit or withdraw collateral funds and to set or adjust the allowances to the collateralizable contracts. The collateralizable contracts mainly interact with CollateralVault on behalf of their users to reserve or release collateral and to claim reserved collateral (i.e., to actually withdraw collateral to the collateralizable contract). A collateralizable contract may also have its own account within the CollateralVault and manage the funds that are accounted as its own. A system fee is charged upon transferring funds out of the CollateralVault contract, essentially upon users' withdrawals and collateralizable contracts' claims.

The TimeBasedCollateralPool contract is a collateralizable contract within the Anvil ecosystem which interacts with CollateralVault. It functions as a staking pool imposing a delay upon exit. Liquidity providers are issued shares of the pool proportional to their staked amount. Staked funds may be withdrawn from the pool at any time by an account with the "claimant" role. The exit delay imposed by the pool ensures a minimum amount of time when the staked funds are available for the claimant. The claim reason is not stated nor verified on the pool contract and is considered to be handled separately, either off-chain or by an external contract. In addition, the incentives for the liquidity providers to stake funds are not part of the pool's contract and are expected to be managed outside of the Anvil protocol.

Any token that is supported by the CollateralVault contract may be staked in pools. The pool's shares are not tokens and are non-fungible. For each token, they only represent a balance of the user's account proportional to the account's participation in the pool. When funds are claimed by the claimant, the value represented by the shares decreases along with the pool's balance. This can reach a point where the shares' value is highly diluted so that any new staking action fails due to an integer overflow during the calculation of the amount of shares to be issued. For this reason, it is possible that the pool is "reset", meaning that all tokens are unstaked and the users are required to either stake them again or withdraw them. Immediately after a reset, shares are again issued in 1:1 proportion to the staked amount. A reset can happen either when the claimable pool balance is zero or a dust amount, or when the resetter role explicitly calls the resetPool function.

Anvil Governance

The protocol's governance utilizes the Anvil token as the voting token. The total supply of the Anvil token is planned to be 100 billion tokens, a portion of which will be minted to the token contract deployer while the rest will be distributed via an airdrop procedure handled by the Claim contract. The airdropped amounts will be subject to a vesting period, after which the beneficiaries can claim them. However, during the vesting period, the beneficiaries will be able to prove their balance with a Merkle proof and delegate the corresponding voting power.

In order to implement this feature of delegating voting power of tokens that are not yet in the beneficiaries' possession, the governance token has been implemented as a modified version of the OpenZeppelin Votes contract. Most importantly, the functionality associated with transferring the voting power has been changed so that the proven yet unclaimed token balance can also be accounted for votes delegation. After a certain point in time, the Claim contract's owner is able to rescue the unclaimed tokens that remain in the contract but cannot rescue the tokens that have been proven yet are unclaimed.

The governance proposals are considered to have reached quorum if at least 4% of the Anvil token's total supply has participated in the voting process. Successful proposals are subject to a timelock delay before their execution in order to guarantee a minimum amount of time for users to react in case of unfavorable proposal actions. The governance contract is upgradeable but is owned by the governance's timelock itself. As such, a voting procedure and a delay are also ensured in case of important governance changes.

Security Model and Trust Assumptions

The protocol defines a number of privileged entities which are able to make crucial decisions or modify sensitive parameters of the system that affect its overall security. During the audit, we assumed that all of these entities would behave honestly and in the best interests of the users and the overall ecosystem. Wherever possible, some specific on-chain restrictions have been suggested which we included as lower-severity issues in this report.

The privileged entities or roles and their corresponding sensitive operations are described below.

  • The owner of the CollateralVault contract is able to set the withdrawal fee ratio (up to 10%), grant or revoke approval for a collateralizable contract, enable or disable a collateral token, and withdraw the fees accrued by transferring the collateral tokens to an arbitrary address of their preference.
  • The CLAIMANT_ROLE in the TimeBasedCollateralPool contract is able to withdraw an arbitrarily large portion of the pool's balance. It has been assumed that the claim reason is well-defined and understood by all participating parties. It has also been assumed that the verification of the claim reason is going to be managed by a separate contract or system.
  • The CLAIM_ROUTER_ROLE in the TimeBasedCollateralPool contract is able to set the destination account for claims. It has been assumed that this role behaves in the claimant's interest.
  • The RESETTER_ROLE in the TimeBasedCollateralPool contract is able to reset the pool at will and under no restriction. It has been assumed that the pool is going to be reset only when necessary (i.e., when the pool's units have been highly diluted due to a large claim). Otherwise, it is possible that the vesting period is not respected as the minimum funds availability period for the claimant, possibly resulting in a DoS for the claim actions.
  • The deployer of the Anvil governance token is immediately minted a portion of the token's total supply. It is assumed that this portion is reasonable and will be handled with respect to the ecosystem's needs.
  • The owner of the Claim contract is able to call the initialize function more than once and (re)set sensitive parameters of the airdrop procedure such as the vesting period and the rescue delay. In addition, the assigned value of the rescue delay is important to guarantee that users are given enough time to claim their ownership over their airdropped balance. It has been assumed that the owner will act in accordance with the users' interests and that the rescue delay will be set to a reasonable point in time.
  • The owner of the AnvilGovernorDelegator contract, which is the proxy contract for governance, is able to upgrade the governance implementation contract. The Ampera team is planning to assign the governance's timelock contract as the owner, essentially having the governance being the owner of itself. In this way, any implementation upgrade is going to undergo the governance proposal procedure.
 

Critical Severity

Proposals Can Reach Quorum Without Any Votes

The minting of Anvil tokens does not update the total voting power recorded in the _totalCheckpoints struct, hence _totalCheckpoints is always empty. This empty value is used to calculate the past total supply of tokens in the getPastTotalSupply function of the AnvilVotes contract, incorrectly indicating the total supply as zero. The getPastTotalSupply function is called in the GovernorVotesQuorumFractionUpgradeable contract to calculate the quorum, which makes the returned quorum always be equal to zero. If the quorum is zero, proposals would reach quorum even without anyone voting on them. Once the proposal deadline has passed, the proposal can be executed. The only way to prevent a malicious proposal from getting executed is to have enough votes against it.

Since the minting of Anvil tokens is restricted to the constructor and there is no possibility of burning Anvil tokens, consider modifying the getPastTotalSupply function so that it returns the total supply of Anvil tokens for the correct calculation of quorum.

Update: Resolved in pull request #281 at commit e1352ca.

High Severity

Funds May Be Locked Inside TimeBasedCollateralPool

Users' deposits are represented by units instead of tokens in the TimeBasedCollateralPool contract. This is because any amount of staked tokens may be claimed at any time by an account with the claimant role and the total amount of tokens inside the pool will decrease in such a case. As such, the users will not be able to receive their full deposit when they unstake their tokens. Instead, they will receive the amount of tokens proportional to their unit's balance. All the unit-related calculations are performed by the calculateProportionOfTotal function of the Pricing library.

Every time users stake their tokens, they receive the amount of units proportional to their contribution to the entire token pool balance. This, combined with the claims that may happen over time, may drastically increase the amount of units needed to represent a single token. For this reason, the reset functionality has been introduced in order to manually release all tokens staked in the pool. This is so that the balance between tokens and units is brought back to a 1:1 ratio when users stake their tokens again. However, there are possible scenarios where users are allowed to deposit their tokens but cannot unstake these tokens later on due to the Integer Overflow exception. One such scenario is described below.

Assume that there is some amount of tokens deposited into the pool, but the units are diluted. For example, let's say that for a token with 18 decimals, the pool holds a total of 1e26 units, but only a dust amount of tokens due to claims. It can either naturally happen over time, or by way of an attack where there is a transaction issued by an account with the claimant role in order to claim the entire collateral, but an attacker front-runs it and deposits 1 wei of the token into the pool. This will avoid resetting the pool and will quickly dilute the units.

Afterward, another user deposits some 1e8 tokens to the pool and receives a very large amount of units ~1e52. After some time, they want to withdraw their assets so they schedule a withdrawal. When the withdrawal is finally processed after two epochs, the user will not be able to claim it as this line will revert. This is because an attempt will be made to multiply two very large numbers (1e26 and 1e52) and an integer overflow will occur. It will also revert for every other user, as the _unlockEligibleTokenContractPendingUnstakes function is being called in order to process any user's withdrawals.

In particular, users will not be able to call any function with the withEligibleAccountTokensReleased modifier. The only way to recover from such a situation would be to reset the pool. However, the user who deposited 1e8 tokens after the units became diluted will still not be able to withdraw their assets, as this time, this calculation will cause an overflow. This will lock the user's funds in the protocol.

Consider verifying that integer overflow will not happen for future withdrawals each time users stake their tokens.

Update: Resolved in pull request #275 at commit 523f534.

Users May Lose The Ability to Transfer Their Tokens

Whenever a change is made to the voting power of any user, the _moveVotingPower function is called. It handles several different scenarios, such as a voting power change during token transfer or during delegations. Another scenario involves claiming the tokens from the Claim contract. Before doing that, users have to prove their balance inside the Claim contract. Although the tokens are vested over time, whenever a user proves their balance, their delegatee immediately receives the voting power from the tokens to be claimed. Because of that, when they finally receive the vested tokens, the voting power of their delegatee does not change as it has already been accounted for in the past.

However, it is possible to execute the code responsible for handling claims during normal token transfer. It can happen when a user transfers tokens to any account while having a non-zero claimable balance and the Claim contract as their current delegatee. In such a case, during the tokens transfer, the _moveVotingPower function will be executed with claimContractAddress as the second argument, and since the user has a non-zero amount of claimable tokens, this check will pass. In order to illustrate the possible consequences of this behavior, consider the following attack scenario where there are two users: Alice and Bob.

Alice has two accounts: one having a non-zero balance and some proven unclaimed tokens to claim from the Claim contract, and a second account with a zero token balance that has Bob as its delegatee. Bob has a non-zero balance in his account. Now, Alice can:

  1. Call proveAndDelegate from her first account and specify the Claim contract as the delegatee.
  2. Transfer her balance to her second account. Since Alice's first account delegates to the Claim contract and its claimable balance is non-zero, no change will be made to the voting power of her second account's delegatee.
  3. Change the delegatee of her first account to itself and transfer the tokens back from her second account to her first account. As a result, the second account's delegatee, Bob, will lose a corresponding amount of his voting power.

Consequently, Bob will not be able to transfer his tokens in the future as an integer underflow will occur during the attempt to decrease his voting power.

Consider not allowing users to delegate to the Claim contract and handling the accounting of the accountProvenUnclaimedAmount outside the _moveVotingPower function.

Update: Resolved in pull request #279 at commit a6cac6a.

Medium Severity

Risk of Storage Collision in Proxy Contract

The AnvilGovernorDelegator.sol proxy contract does not follow the ERC-1967 standard for proxy storage slots. The contract stores the implementation address in a non-random slot, which can lead to a storage collision with the logic contract if some state variables are in use.

The AnvilGovernorDelegate.sol logic contract does not have any state variables using the storage slots. However, if the logic contract upgrades in the future and introduces a state variable, the proxy contract storage will collide, leading to a contract takeover due to the collision of the _owner variable or making the contract entirely inoperable due to the collision of the _implementation variable.

Consider following the ERC-1967 standard and utilize unstructured storage approaches for proxy implementations.

Update: Resolved in pull request #298 at commit 0a5cb12.

Low Severity

It Is Possible to Call poolCollateral for a Disabled Token

The users of the CollateralVault contract can only interact with certain whitelisted tokens. These tokens may be enabled or disabled by the owner of the CollateralVault contract through a call to the upsertCollateralTokens function. For a disabled token, it is not possible to create reservations or to deposit more funds, and only operations related to withdrawals are supported.

However, the poolCollateral function does not check if the collateralToken is enabled. Hence, a collateralizable contract can pool the existing collateral of a disabled token, thereby transferring funds between the user's and the collateralizable contract's accounts within CollateralVault.

Consider verifying that the poolCollateral function can only interact with enabled tokens.

Update: Resolved in pull request #303 at commit b9f99cf.

Unused Code

In the _moveVotingPower function of the Anvil contract, the integer sign of the fromVotingUnitDelta variable makes a distinction between the cases where voting units should be subtracted from, or added to, the from address.

However, this distinction is redundant since there is no case where fromVotingUnitDelta is a positive value. For this reason, there is also no need to cast fromVotingUnitDelta to a negative integer value. This variable could be declared as an unsigned integer and subtraction could always be performed when it is assigned a non-zero value.

Consider declaring the fromVotingUnitDelta variable of the _moveVotingPower function as an unsigned integer in order to considerably simplify the code.

Update: Resolved in pull request #304 at commit d8a885c.

Allowance Changes Are Allowed to Non-Whitelisted Collateralizable Contracts

The depositAndApprove, modifyCollateralizableTokenAllowance, and modifyCollateralizableTokenAllowanceWithSignature functions of the CollateralVault contract do not validate whether the collateralizable contract with the given address is approved. Instead, the internal functions are immediately called, assuming that any necessary input validation has been already performed on the caller's side. Although no specific way to exploit this lack of validation was identified, it is possible that invalid data is inserted into the accountCollateralizableTokenAllowances mapping.

Consider always validating that _collateralizableContractAddress belongs to an approved collateralizable contract or only permitting decreasing the allowance of a formerly enabled collateralizable contract for consistency and clarity.

Update: Resolved in pull request #305 at commit 6ca3a49.

Inconsistent Starting and Ending Epoch Timepoints

In the TimeBasedCollateralPool contract, the getCurrentEpoch function is exclusive of the next epoch's starting second, whereas according to the getEpochEndTimestamp function, the ending timestamp for epoch i is equal to the starting timestamp for epoch i+1.

For consistency and clarity, consider excluding the next epoch's starting timestamp in the getEpochEndTimestamp function.

Update: Resolved in pull request #306 at commit 5b58bbb.

Lack of Input Sanity Checks

Throughout the codebase, there are several instances of missing or insufficient function argument validation. Some instances are listed below:

  • Since the users are allowed to withdraw tokens from a disabled token address, the withdrawal of tokens from CollateralVault does not verify whether the _tokenAddress is approved by governance or not. However, if the input _amount is zero, the contract can make a safe transfer to any arbitrary token address and emit a misleading FundsWithdrawn event.

  • While the docstring within the _moveVotingPower function states that the _destination address provided for rescuing tokens in the Claim contract should not have a claimable balance, this condition is not checked in the codebase. If the _destination address has an initial proven balance, rescue of tokens may revert in case the _tokenAmount is greater than their delegatorProvenUnclaimedUnits.

  • There is no check ensuring that the _tokens and _amounts arrays have the same length in the claim function of the TimeBasedCollateralPool contract.

In order to reduce the attack surface of the codebase, consider implementing all the missing checks.

Update: Resolved in pull request #307 at commit 1c3402a.

Ownership Can Be Renounced

The owner of the Claim contract can renounce ownership by calling the renounceOwnership function. If the ownership is lost, the ownerRescueTokens function cannot be called, prohibiting the recovery of tokens locked in the contract.

Consider overriding the renounceOwnership function to revert if an attempt to renounce ownership is made when there are tokens left in the contract.

Update: Resolved in pull request #308 at commit df852c8.

Users Will Lose Tokens if They Attempt to Transfer Them to Themselves

The transferCollateral function may be used in order to transfer between accounts those tokens which have already been deposited into the vault. This transfer does not cost any fees and it is performed by adding the target amount of tokens to the recipient's account and by subtracting them from the sender's account. However, the value used for calculating the new sender's balance is cached before the recipient's balance is calculated, which means that if sender and recipient are the same, their accountBalances entry will decrease by the token amount instead of staying the same.

Consider not modifying a user's balance upon a transfer when the sender is the same as the recipient.

Update: Resolved in pull request #309 at commit ca00376.

depositAndStake Could Fail Due to Front-Running

By calling the depositAndStake function of the TimeBasedCollateralPool contract, a user can deposit funds into the CollateralVault contract and also stake them in a single transaction. To trigger the deposit action, the collateralizable contract must have been granted a non-zero allowance by the user. As an alternative, the user can provide a signed message to increase the corresponding allowance and thus make the whole transaction possible. However, an attacker may front-run a depositAndStake transaction containing a signed message and make it fail as follows:

  1. A user submits a depositAndStake transaction for which they have a signed a message.
  2. The attacker front-runs the user's transaction and uses it to call modifyCollateralizableTokenAllowanceWithSignature successfully.
  3. The user's depositAndStake call fails because the signed message has already been used, thereby increasing the nonce.

The attacker has no immediate profit but can ruin the user's experience.

Consider requiring a separate message for allowing a collateralizable contract to trigger a deposit action on behalf of a user.

Update: Resolved in pull request #266 at commit 615c922.

Floating Pragma

Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled. Throughout the codebase, there are numerous contracts that use the solidity ^0.8.20 floating pragma directive.

Consider using fixed pragma versions.

Update: Resolved in pull request #310 at commit 682ae20.

Notes & Additional Information

Unused Named Return Variables

Named return variables are a way to declare variables that are meant to be used within a function's body for the purpose of being returned as that function's output. They are an alternative to explicit in-line return statements. Throughout the codebase, there are unused named return variables:

Consider either using or removing any unused named return variables.

Update: Resolved in pull request #311 at commit 5c3c9b1.

Unnecessary Cast

Within the Claim contract, the uint256(amount) cast is unnecessary.

To improve the overall clarity, intent, and readability of the codebase, consider removing unnecessary casts.

Update: Resolved in pull request #312 at commit af85da8.

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 proves 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 these, it becomes easier for the maintainers of those libraries to contact the appropriate person about the problem and provide mitigation instructions. The contracts of the audited codebase do not have a security contact.

Consider adding a NatSpec comment containing a security contact above the contract definitions. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.

Update: Resolved in pull request #313 at commit 04a6952.

Hardhat console Import Present

There is a Hardhat console import in line 15 of AnvilGovernorDelegate.sol.

To improve the overall readability of the codebase, consider removing Hardhat debugging imports.

Update: Resolved in pull request #314 at commit ed1926b.

Duplicated Code

Throughout the codebase, there are several instances of duplicated code:

Consider refactoring the code so that the instances of duplicated code are removed. This will help make the codebase more concise and less error-prone.

Update: Resolved in pull request #315 at commit a453a51.

Misleading Documentation

Throughout the codebase, there are several instances of misleading or incorrect documentation. Some examples are listed below:

  • The documentation of the _unlockEligibleTokenContractPendingUnstakes function suggests that the function iterates through the tokenContractPoolBalances array. However, such an array does not exist and the function does not iterate through any data.
  • The documentation inside the _authorizedModifyCollateralizableTokenAllowance function suggests that _byAmount will always be negative at that point, but it can also be equal to 0.
  • The documentation of the getClaimableCollateral function suggests that the function is not a view function, but it is declared as such.
  • The inherited documentation for the getAccountPoolBalance function of the TimeBasedCollateralPool contract is not clear and makes no specific comment regarding the notion of an account's "balance" in association with vested or unvested stakes.

Consider fixing the documentation in order to improve the overall clarity and readability of the codebase.

Update: Resolved in pull request #316 at commit 9b15022.

Code Clarity Suggestions

Throughout the codebase, a number of instances were identified where code clarity could be improved:

  • Since the _proof argument of the proveInitialBalance function should not be edited in any way, consider passing it to the _verifyInitialBalanceOrRevert function as calldata instead of memory in order to avoid copying it to memory. In addition, consider using the verifyCalldata function of the MerkleProof library.
  • Throughout the TimeBasedCollateralPool contract, the condition epoch > 0 && epoch < currentEpoch is checked multiple times. Consider putting it into a helper function in order to reduce code duplication and make the code more readable.
  • Several functions throughout the CollateralVault contract require that the passed token or collateralizable contract address are enabled by the owner. Consider using modifiers for these checks to make the requirements clearer to the reader.

Update: Resolved in pull request #317 at commit 6965e70.

Multiple Initializations of Claim Are Possible

The Claim contract can be initialized multiple times by the owner, allowing them to modify sensitive parameters (which should not be changed) such as balanceRoot, vesting duration, and ownerRescueTimestamp.

Consider allowing the initialize function to be called only once in order to guarantee that the parameters will not be changed after they are initially set.

Update: Resolved in pull request #318 at commit 9e1bcec.

Using uint Instead of uint256

Throughout the codebase, there are uses of uint as opposed to uint256:

In favor of explicitness and style consistency, consider replacing all instances of uint with uint256.

Update: Resolved in pull request #319 at commit 246426d.

Unused Errors

Throughout the codebase, there are several instances of errors which are declared, but not used anywhere. Some examples include:

In order to improve the readability of the codebase, consider removing all unused errors.

Update: Resolved in pull request #320 at commit 4a6f0bc.

Typographical Errors

Throughout the codebase, there are several instances of typographical errors present. Some examples are listed below.

In the Pricing contract:

  • In line 22, "credited token token" should be "credited token".
  • In line 40, "give" should be "given".
  • In line 80, "amountBeforeFee function" should be "amountWithFee function".

In the TimeBasedCollateralPool contract:

  • In line 119, the word "for" is redundant and should be removed.
  • In line 517, the word "grated" should be "granted".
  • In line 881, the second "the" should be removed.

In the AnvilVotes contract:

  • In line 117, "it no longer used" should be changed to "it is no longer used".

In the AnvilGovernorDelegate contract:

  • In line 18, "AnvilGovernorDelegate" should be changed to "AnvilGovernorDelegator".

In order to improve the readability of the codebase, consider fixing all instances of typographical errors.

Update: Resolved in pull request #321 at commit 1218798.

Todo Comments in the Code

During development, having well-described TODO/Fixme comments will make the process of tracking and solving them easier. Without this information, these comments might age and important information for the security of the system might be forgotten by the time it is released to production. These comments should be tracked in the project's issue backlog and resolved before the system is deployed. The Following TODO/Fixme comment was found:

Consider removing all instances of TODO/Fixme comments and instead tracking them in the issues backlog. Alternatively, consider linking each inline TODO/Fixme to the corresponding issues backlog entry.

Update: Resolved in pull request #322 at commit 8816b61.

Inconsistent Use of Named Returns

Throughout the codebase, there are multiple instances where contracts have inconsistent usage of named returns in their functions. A non-exhaustive list of such instances is given below:

To improve readability, consider being consistent with the use of named returns throughout the codebase.

Update: Resolved in pull request #311 at commit e204a18.

Lack of Indexed Event Parameters

Throughout the codebase, several events do not have indexed parameters:

To improve the ability of off-chain services to search and filter for specific events, consider indexing event parameters.

Update: Resolved in pull request #323 at commit 8ea9d51.

Duplicate Imports

Throughout the codebase, there are several instances of duplicated imports:

Consider removing duplicate imports to improve the overall clarity and readability of the codebase.

Update: Resolved in pull request #324 at commit 628feb1.

Unused Imports

Throughout the codebase, there are several instances of unused imports:

In order to improve the readability of the codebase, consider removing all unused imports.

Update: Resolved in pull request #324 at commit 628feb1.

Negative Allowance in CollateralVault Can Be Counterintuitive

The CollateralVault contract allows users to set negative allowances for collateralizable contracts. While it provides flexibility to maintain the allowance, there are certain scenarios where this feature can be counterintuitive and create confusion for the user, thereby adversely affecting the user experience. For instance:

  • Consider a scenario where a user initially sets an allowance of amount A to a collateralizable contract and this amount gets reserved. Then, the user decides to zero out the overall allowance by setting the allowance to -A. Later, the user wishes to grant allowance A to the same collateralizable contract for another reservation, but due to confusion, only adds amount +A, resulting in a zero allowance and causing the reservation to fail.
  • Negative allowances remain if the reserved amounts are claimed, even when there is no chance for the allowance to increase automatically by a release action. In this scenario, the user who previously set the allowance to a negative value could fail to increase the allowance to a positive value due to confusion.

In addition to the instances above, this feature makes the code more complicated and negatively impacts the readability of the codebase.

Consider using a single-use, non-negative allowance mapping to track allowances.

Update: Resolved in pull request #267 at commit b25ec58.

Add Functionality to Cancel Signed Messages

By calling the depositFromAccount function of the CollateralVault contract, a collateralizable contract that has zero or negative allowance can use a message signed by a user to increase its allowance for the user's account and deposit funds directly from the user's wallet.

Given that this functionality is highly sensitive, consider adding a cancelModifyCollateralizableTokenAllowanceWithSignature function which the user could call to cancel the signed message (e.g., by increasing their nonce).

Update: Resolved in pull request #325 at commit 343b09f. The Nonces contract has been replaced by the custom SignaturesNonces contract which enables separate nonces per message typehash and supports cancelling batches of signed messages that have not been verified yet.

Counterintuitive depositFromAccount Functionality

The users of the CollateralVault contract can deposit funds into their account and set a specific allowance amount for one or more collateralizable contracts. In this way, each collateralizable contract is able to handle at most the allowed amount on the user's behalf, whereas the total deposits may be less than the total allowance made to the collateralizable contacts.

The depositFromAccount function of the CollateralVault contract can be called from a collateralizable contract in order to increase an account's deposited funds by transferring funds directly from the user's wallet. Thus, if a user has given an unlimited or large enough allowance to CollateralVault, the collateralizable contract is able to successfully perform a deposit on the user's behalf, unless the collateralizable's allowance is zero or negative.

In essence, the depositFromAccount function equates the user's allowance made to the collateralizable contracts with the allowance made towards the CollateralVault contract. However, the fact that one is related to the user's wallet and the other to the user's account in CollateralVault creates a qualitative difference between these two kinds of allowances in the protocol. The allowance towards CollateralVault seems to simply offer flexibility and gas savings so that the user can allow a very large amount only once and then decide to what extent it will be used by triggering deposit actions in the future.

In addition, the fact that a user may set a larger sum of allowances to the collateralizable contracts compared to the total funds deposited in CollateralVault seems to be a feature which allows the user to control the total risk taken. The depositFromAccount function does not respect this feature and seems to be counterintuitive for the user. It is also possible that a collateralizable contract transfers a significant amount from a user's wallet to the CollateralVault even if it has insufficient allowance to use it. However, the user would still be obliged to pay the system's fee to withdraw the funds back to their wallet.

Consider defining a specialized message typehash for permitting deposit actions and always require a signed message in order to execute depositFromAccount.

Update: Resolved in pull request #266 at commit be2f3b8.

Recommendations

Compatibility With Tokens That Extend ERC-20

The owner of the CollateralVault contract is responsible for enabling or disabling the token contracts that can act as collateral. There are no specific guidelines or requirements for the enabled tokens other than they are expected to follow the ERC-20 standard. However, there are some extensions of the ERC-20 standard that should not be enabled for specific reasons:

  • Fee-on-transfer tokens. These tokens will result in erroneous accounting upon deposit actions as the amount received by the vault will be lower than the provided deposit amount.
  • Rebasing tokens. If the contract's balance is increasing after a rebase then the extra amount will be eventually held by the CollateralVault contract as fee which is unfair to the depositors. On the other hand, if after a token's rebase the contract's balance is decreasing, then the whole accounting is against the protocol and any depositor can benefit until all the contract's funds are drained.
  • Upgradeable token contracts. It is generally a risk to whitelist upgradeable contracts since their implementation might be altered.

We have also considered the case of ERC-777 tokens but could not identify a specific scenario where the transfer hooks would introduce an attack vector, at least under the current implementation of the CollateralVault and TimeBasedCollateralPool contracts.

Consider explicitly documenting that instances of the tokens listed above should not be enabled inside CollateralVault.

Factory Contract for ITimeBasedCollateralPool Contracts

In the stakeReleasableTokensFrom function of the TimeBasedCollateralPool contract, if the ITimeBasedCollateralPool _pool argument is not equal to the contract's address then an external call to _pool is made. Essentially, it is expected that the _pool contract implements similar functionality so that the releaseEligibleTokens function releases the vested unstaked tokens to the user's account in CollateralVault. In this way, the released funds can be reserved again in order to be staked in Anvil's TimeBasedCollateralPool.

However, there is no guarantee regarding the actual implemented functionality of the called contract. While no specific vulnerabilities have been identified, calling arbitrary code increases the attack surface.

Consider adding a factory contract which deploys pool contracts as per the TimeBasedCollateralPool implementation. In this way, it is possible to only allow external calls to pool contracts that have been deployed by the factory contract, which is a guarantee about the code executed. 

Conclusion

The Ethereum-based Anvil Protocol aims to manage collateral transparently via the CollateralVault contract. Using this contract, users can deposit and withdraw collateral and set allowances for collateralizable contracts. Anvil's governance uses the Anvil token for voting, with a total planned supply of 100 billion tokens distributed mainly through airdrops subject to vesting periods. Voting delegation for these tokens is enabled using a modified OpenZeppelin Votes contract.

One critical-severity and two high-severity issues have been discovered, mostly due to important modifications of external library contracts for the governance token. Despite the fact that a high-severity issue was discovered inside the TimeBasedCollateralPool contract, it nonetheless handles considerably subtle accounting properly.

The Ampera team has sufficiently tested certain parts of the audited codebase, including fuzzing for the TimeBasedCollateralPool and the governance's Claim contract. However, we recommend enhancing the test suite for the governance token contracts which contain the modified library code. There are some instances where the code can be simplified or the functionality could be made more intuitive for the common user. This has led us to including a number of recommendations in this report.

The Ampera team provided us with extensive documentation for the protocol and has been responsive and engaging in discussions with the auditors during the audit period.

 

Request Audit

 

Related Posts