QA Report #194
Labels
bug
Something isn't working
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
sponsor acknowledged
Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Low & QA report
Relevant portions of code are marked with
@audit
tags.Low
Low-01: No check for interface implementation for newly added
extraRewards
inBaseRewardPool.sol
Description:
The
rewardManager
of aBaseRewardPool
can add new tokens to beextraRewards
of a pool that are distributed to users in addition to the normalstakingToken
. However since there is no check that the newly added token address implements the expected interfaceIReward
- notably, thewithdraw
andstake
functions amoung others - it's possible for the admin to mistakenly add an incompatible address to theextraRewards
list, and for subsequent external calls to it to fail.Since the handling of the
extraRewards
tokens is processed along withstakingToken
, users will not be able to perform critical operations on the pool like withdrawing or staking if one of the extra rewards tokens is not implemented correctly.The
rewardManager
can only callclearExtraRewards()
which deletes the entire array, and they will have to re-add all of them again.POC:
Function to add new extra rewards
Function to withdraw
stakingToken
andextraRewards
if available:Mitigation steps
Consider using ERC165 to check that the address implements the expected interface, or allow the rewardManager to remove specific tokens from the
extraRewards
list by using swap and pop or another data structure.Low-02: Calls to
AuraVestedEscrow:fund()
can be frontrun with bogus valuesDescription:
The
fund()
function is used to initialize the contract after creation with the appropriate amount of rewardTokens. Since there are no permissions required to call it, a malicious user can listen for calls tofund()
and frontrun it with their own transaction. They can pass in two empty arrays which will require them to transfer 0 rewardTokens to the contract. Thus, theinitialized
flag is set totrue
and no one else can callfund()
again.The contract is unusable and it was at 0 cost to the attacker. Now the team must pay txn fees for redeployment.
POC
Mitigation
Consider only allowing admins to fund the contract, or choose to deploy and fund using private relayers.
QA
QA-01: Emit event for adding a new rewardsToken to AuraLocker
Add a
NewRewardToken
event here to be in line other emitted events in the rest of the codebase.Specific code link here.
QA-02: Move
AuraLocker.sol:setApprovals()
from the ADMIN section to the Actions section as it is not priviledged.QA-03: No setter function for
penaltyForwarder
inAuraMerkleDrop.sol
There is no function allowing the penaltyForwarder to be changed after contract creation. Additionally, there is no check in the constructor that the supplied
penaltyForwarder
is not the 0 address. Thus, if the value is incorrectly set on creation,forwardPenalty()
will revert and there will not be a way to fix the issue without redployment. It's helpful to always have a way to change state variables that are critical to the contract's functionality (with the correct permissioning of course).QA-05: Emit event for new
extraRewards
added inBaseRewardPool.sol
Keep it inline with the rest of the codebase where events are emitted with important state changes. Also will help in transparency for users.
QA-06: Emit event for
extraReward
withdraw inBaseRewardPool.sol:withdraw()
QA-07: Check return value for
stake(balance)
inBaseRewardPool.sol:stakeAll()
If stake() will expected to return false in the future, it pays to check it here before it is missed. Or do
return stake(balance)
.QA-07:
donate()
function inBaseRewardPool.sol
does not return a value despite function signaturereturn true
to follow the pattern in the rest of the codebase.QA-08: @param for
seal
missing in constructor inBoosterOwner.sol
Real nitpicky but
QA-09: Change events emitted for ownership transfer in
BoosterOwner.sol
Currently the contract emits
TransferOwnership
when thependingOwner
is updated, andAcceptedOwnership
when the pendingOwner accepts it.I think a more logical nomenclature would be to emit
PendingOwnerUpdated
whentransferOwnership()
is called, and emitAcceptedOwnership
when thependingOwner
is accepted as well asPendingOwnerUpdated(0x0)
afterwards.(This is the same pattern used in Seaport)
The text was updated successfully, but these errors were encountered: