QA Report #275
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
Low
Missing parameter validations in
SpeedBumpPriceGate#addGate
Callers of
addGate
can create price gates with a zero price floor (allowing users to claim free tokens), and zeropriceIncreaseDenominator
(causing price calculation to revert with a divide by zero error).SpeedBumpPriceGate#addGate
Suggestion: Validate that
priceFloor
andpriceIncreaseDenominator
are nonzero.VoterID
token can be minted to the zero addressVoterID
tokens can be minted to the zero address inVoterID#createIdentityFor
.VoterID#createIdentityFor
Suggestion: validate
thisOwner
increateIdentityFor
:VoterID
token can be minted to non-ERC721 receiversVoterID
tokens can be minted to non-ERC721 receivers inVoterID#createIdentityFor
.VoterID#createIdentityFor
Suggestion: check
checkOnERC721Received
increateIdentityFor
. This callback introduces a reentrancy vector, so take care to ensure callers ofcreateIdentityFor
use a reentrancy guard or follow checks-effects-interactions:VoterID
ownership can be transferred to the zero addressThe
owner
ofVoterID
can be intentionally or accidentally set toaddress(0)
, which would permanently deny access toownerOnly
protected functions.VoterID.sol#L151-L155
Suggestion: Validate that
newOwner
is notaddress(0)
insetOwner
:Additionally, consider implementing two-step ownership transfers.
Prefer two-step ownership transfers
If the
owner
ofVoterID
accidentally transfers ownership to an incorrect address, protected functions may become permanently inaccessible.VoterID.sol#L151-L155
Suggestion: handle ownership transfers with two steps and two transactions. First, allow the current owner to propose a new owner address. Second, allow the proposed owner (and only the proposed owner) to accept ownership, and update the contract owner internally.
balanceOf
does not revert on zero address queryAccording to the ERC721 spec and the natspec comment in the code,
VoterID#balanceOf
should revert when called with the zero address, but it does not:VoterID.sol#L168-L175
Suggestion: Validate that
_address
is notaddress(0)
inbalanceOf
:Prefer
safeTransfer
andsafeTransferFrom
for ERC20 token transfersConsider using OpenZeppelin's
SafeERC20
library to handle edge cases in ERC20 token transfers. This prevents accidentally forgetting to check the return value, like the example inMerkleVesting#withdraw
.Potential changes:
PermissionlessBasicPoolFactory.sol#L144
PermissionlessBasicPoolFactory.sol#L198
PermissionlessBasicPoolFactory.sol#L230
MerkleVesting.sol#L89
MerkleResistor.sol#L121
MerkleResistor.sol#L204
MerkleDropFactory.sol#L77
MerkleDropFactory.sol#L107
QA/Noncritical
Move
require
check to top of functionThe
require
check inPermissionlessBasicPoolFactory#addPool
comes after several state changes. Consider moving it to the top of the function to follow the checks-effects-interactions pattern.PermissionlessBasicPoolFactory.sol#L112
Replace inline assembly with
account.code.length
<address>.code.length
can be used in Solidity >= 0.8.0 to access an account's code size and check if it is a contract without inline assembly.VoterID#isContract
Suggestion:
VoterID#transferFrom
does not distinguish nonexistent tokens from unapproved transfersUnlike other common ERC721 implementations,
VoterID
does not distinguish an attempt to transfer a nonexistent token from an unapproved transfer:VoterId#transferFrom
Consider checking that a token exists in
isApproved
to distinguish attempts to transfer nonexistint tokens. (See OpenZeppelinERC721#_isApprovedOrOwner
for an example).Emit events from privileged operations
Consider adding events to protected functions that change contract state. This enables you to monitor off chain for suspicious activity, and allows end users to observe and trust changes to these parameters.
VoterId#setTokenURI
MerkleIdentity#setManagement
MerkleIdentity#setTreeAdder
MerkleIdentity#setIpfsHash
Incomplete natspec comment
The
@notice
natspec comment onVoterID
is incomplete.The text was updated successfully, but these errors were encountered: