Ampleforth's Token Geyser v2

CertiK12 Feb 2021Files named in the report

Audit

CertiK reviewed AMPL (Ampleforth's Token Geyser v2) on 12 Feb 2021: 23 findings, 0 critical; 22 marked fixed, 1 open.

No open critical findings

Open the report Open AMPL’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings23

Statuses are the security firm’s own, from its report.

Severity	Open	Fixed
Critical	0	0
High	0	1
Medium	0	2
Low	1	19

HighInvalid Balance Sheet EvaluationFixed

The checkBalances function is meant to iterate over all locks existent on the vault and ensure that the locked balances do not exceed the amount of tokens held by the vault. The issue with the current implementation is that the loop iteration between L239 and L244 checks the balances of the locks sequentially whilst the same token can exist under two different lock IDs within the _lockSet set. For example, if two different Geyser s rely on the same vault, the same token will be "locked" under two different lock IDs, for the sake of this example let's consider that value to be 100 . If I were to maliciously call either externalCall or externalCallMulti , I would be able to withdraw 100 of the 200 units held by the vault as the two locks created above would each be 100 which would successfully pass the check imposed on L243.

MediumFunction Comment InconsistencyFixed

The stake function comments denote that the totalStakeUnits should be adjusted when a stake occurs. However, the implementation of stake adjusts the units only before submitting the new stake and does not update the totalStakeUnits after, as unstakeAndClaim does.

MediumSignature Validation Race ConditionFixed

The linked functions lock and unlock perform signature validation based on a globally incremented nonce on both functions regardless of the party that is providing the signature. This introduces a race condition whereby a validly signed lock can be invalidated by submitting another validly signed lock with a higher gas fee. This can render the system unusable in a realworld use case with high influx of activity as lock / unlock signature nonces will be overlapping between users.

LowBytecode OptimizationFixed

The linked functions differ in the code they execute by a single variable which is passed as a literal in one implementation and as a variable in the other.

LowConditional OptimizationFixed

The newStakeUnits calculated depend on whether time has passed between the timestamp and the lastUpdate of the _geyser , meaning that if those two are equal no change will occur.

LowDenial-of-Service AttackFixed

Although this particular attack vector requires escalated privileges, it is possible to freeze any unstakeAndClaim invocations by introducing numerous bonus tokens to the _bonusTokenSet thus causing the loop iteration of L998-L1014 to run out-of-gas.

LowDynamic Evaluation of Loop LengthFixed

The linked for loops iterate from 0 until a specified length that is the result of a function invocation on the _lockSet . As the conditional statement is evaluated on each iteration, it is more optimal to store the length evaluation in-memory prior to the loop to optimize the conditional evaluation

LowDynamically Computed Static ValueFixed

The linked lines compute the keccak256 value of string literals.

LowFunction Visibility OptimizationFixed

The linked function is declared as public , contains array function arguments and is not invoked in any of the contract's contained within the project's scope.

LowFunction Visibility OptimizationFixed

The linked function is declared as public , contains array function arguments and is not invoked in any of the contract's contained within the project's scope.

Show 10 more

LowInput SanitizationFixed

The constructor of the PowerSwitch contract transfers ownership of itself to the address provided as input to it. However, no check exists that ensures the owner is non-zero in either the PowerSwitch implementation or parent contracts, such as Geyser , that create instances of it.

LowInput SanitizationFixed

The constructor of the VaultFactory contract accepts a single address argument that remains immutable beyond its assignment and is used as the underlying implementation of spawned instances. However, no check is imposed in the constructor to ensure that it is not accidentally set to the 0x0 address.

LowInsufficient Prevention of AllowanceFixed

The _externalCall implementation is meant to perform a function call that conducts an arbitrary action except from approving another address of an allowance, presumably to ensure that funds aren't exited from the vault after the checkBalances evaluation successfully passes. This check, however, is insufficient as a lot of contracts, including the Ampleforth token itself (AMPL), derive from OpenZeppelin and support the increaseAllowance and decreaseAllowance functions, thus circumventing the check.

LowInversion of if ClauseFixed

The linked if clauses perform a conditional check whereby within the value literal 0 is assigned to the variable declared before the if block. As uint variables are initialized at 0 by default in Solidity, it is possible to invert the if clause and drop the else leg entirely.

LowModifier require To Function CallFixed

In Solidity, modifier s work by essentially wrapping the function they are utilized in with the corresponding code of the modifier , either appending or prepending statements. As require calls with an error message significantly increase the bytecode size and gas cost, it is more optimal to instead have the modifier implementations perform a function call, leading to the require checks not being duplicated and instead being existent on a single location.

LowMutability Specifiers MissingFixed

The linked variables are assigned to only once, either during their contract-level declaration or during the constructor 's execution.

LowMutability Specifiers MissingFixed

The linked variables are assigned to only once, either during their contract-level declaration or during the constructor 's execution.

LowPotential Misbehaviour of the SystemFixed

The externalCallsMulti function iterates and executes all calls provided to it before evaluationg that the balance sheet of the vault is correct and finalizing the function's execution. This allows one to actually withdraw tokens and utilize them prior to returning them in the sequence of external calls performed by the contract which may be an undesired capability of the system.

LowPotentially Misutilized ImplementationFixed

The _truncateStakesArray implementation is meant to remove a number of StakeData members from the input array by initializing a new one in-memory, assigning to it in a sequential fashion and returning the new array copy. However, the function is solely utilized in L617 to remove the last stake of the array.

LowRedundant CodeFixed

The implemented function _unregister is meant to remove an instance from the _instanceSet and emit a corresponding event, however, it remains unutilized throughout the project.

Show 2 more

LowUnnecessarily Convoluted LogicFixed

The linked code segment executes the calculateRewardFromStakes function which truncates the stakes consumed to produce the reward and returns the truncated array. In the code segment linked, the truncated array is not utilized directly and only the last remaining element is used along with the array's length .

LowVariable ShadowingFixed

The linked variable shadows an existing declaration in a parent contract, OwnableUpgradeable , of the owner function that retrieves the owner of the contract.