The Augur team asked us to review and audit a number of components of the Augur Core v2 project. We looked at the code and now publish our results.
The audited commit is 67c0fec95e65fca8d27751f28a6c06e5247978c6 and only the following Solidity smart contracts were included in the audit’s scope:
GnosisSafeRegistry.solWarpSync.solProxyFactory.solAffiliateValidator.solreporting/Affiliates.solreporting/Market.sollibraries/Initializable.sollibraries/token/ERC1155.sollibraries/token/ERC1155Holder.sollibraries/token/ERC1155Receiver.solThe audit was primarily focused on:
GnosisSafeRegistry contract. Note that Augur’s GnosisSafe.sol file was not audited, though it was compared against official, known, Gnosis Safe Multisig wallet implementations to check for compliance.Additionally, we included the Market contract for completeness, as it suffered changes related to the audited features (mostly after the introduction of the new affiliates system). Yet changes related to other integrations, such as DAI, were not considered at all during the audit.
Update: Many of the issues have been addressed or accepted by the Augur team. Our analysis of the mitigations assumes the pull requests will be merged, but disregards any other potential changes to the code base as well as any changes that do not affect the audited functionality.
Next, our audit assessment and recommendations, in order of importance.
The GnosisSafeRegistry contract is intended to be a single on-chain registry to keep track of Augur users’ Gnosis Safe Multisig wallets. The registry is expected to be called upon creation of a Gnosis Safe Multisig so that Augur can quickly look up and verify a user’s wallet. GnosisSafeRegistry allows users to register one or more wallets through the register and callRegister functions. Calls to these functions are expected to always be executed from a user-controlled Gnosis Safe Multisig wallet. Wallets are stored in the accountSafes mapping under the first address listed in the owners array of the GnosisSafe contract. As a consequence, a single owner can have multiple wallets registered in Augur’s GnosisSafeRegistry.
GnosisSafeRegistry is implemented as a registry of proxy contracts pointing to a single known, legitimate, implementation of GnosisSafe. All registered proxies must:
gnosisSafeMasterCopy. This address is queried from the Augur contract during initialization of the registry.Proxy contract created through the ProxyFactory contract.Additionally, to further restrict the type of valid wallets, all registered wallets can only have a single owner and a threshold equal to 1.
In spite of all described restrictions, it is possible for an attacker to register malicious Gnosis Safe Multisig wallets under any owner address. By leveraging Gnosis Safe modules, an attacker can craft, deploy and register Gnosis wallets with any owner address and modules attached. Complying with all requirements programmatically enforced by Augur’s registry, these wallets would appear to be legitimate and victim-controlled, but would actually contain a malicious backdoor that would give the attacker unrestricted control over it. This undermines the fundamental assumption of the GnosisSafeRegistry contract, thus it should be considered a critical vulnerability. A step-by-step proof-of-concept exploit of this vulnerability can be found in this private repository.
At least two sensitive attack vectors have been identified:
getSafe function, the victim would see a valid wallet registered under her address (and she would even be its sole owner). However, this wallet would have attached an attacker-controlled module that would grant the attacker unrestricted privileges over it. Thus, as module actions do not require owners’ signatures, the attacker would be able to trigger any kind of transactions from the wallet without requiring confirmation from its owner (i.e., the victim).In both cases the attack leverages the fact that Augur’s registry tracks wallets under their owner addresses. As explained, in Gnosis Safe Multisig wallets with malicious modules attached, addresses listed as owners can be easily controlled by an attacker. Consider implementing further restrictions to prevent any attacker to register backdoored wallets for any owner address in Augur’s registry. As starting points to be further analyzed by Augur’s development team, we suggest:
getModules public getter.register and callRegister functions is the address listed in the owners array of the GnosisSafe contract. This can be checked by requiring the owner’s signature or a transaction signed by the owner address.Update: Fixed in commit 7787dba1. Now only Gnosis Safe Multisig wallets deployed with known parameters and no modules can be registered.
It is possible for any user to create a malicious Augur market that can arbitrarily decide when to pay affiliate fees (including the 20% that should be directed to the source trader account).
Any attacker can create a Market through the createMarket function of the MarketFactory contract. The _affiliateValidator parameter may be the address of an attacker-controlled contract implementing the IAffiliateValidator interface. This address is then set during initialization of the Market contract without any kind of validation.
Once the recordMarketCreatorFees function of the Market contract is called, the Market contract executes a call to the getAndValidateReferrer function of the Affiliates contract, passing the attacker-controlled affiliateValidator address as the second argument. In turn, the getAndValidateReferrer function will attempt to call a validateReference function in the attacker-controlled affiliateValidator address, providing the two accounts as parameters. At this point, the attacker is in full control of the execution and can arbitrarily determine whether or not the call is successful. If it is not, then the getAndValidateReferrer function will return the zero address. As a consequence, back in the Market contract, the _affiliateAddress local variable will be set to zero, which will prevent paying out the trading fees to the _sourceAccount and accumulating affiliate fees.
It must be stressed that the decision of if and when to pay fees does not currently depend on the Augur protocol at all, but rather on arbitrary logic chosen by the creator of the market in the affiliateValidator address.
Trying to prevent this attack by limiting the gas forwarded to the potentially malicious AffiliateValidator contract does not seem to be a complete solution. Consider putting more strict limitations on the kind of affiliate validator contract that a market creator can set. Ultimately, consider entirely disallowing market creators to set potentially malicious contracts in a market’s affiliateValidator address, and instead deploying and setting legitimate AffiliateValidator contracts programmatically during a Market initialization.
Update: Fixed in commit 8e62ffe8. Anyone can create a new AffiliateValidator instance using the known contract template, and the Market contract ensures the specified validator is one of these instances.
The recordMarketCreatorFees function of the Market contract determines under which circumstances a portion of the market’s creator fees gets distributed to affiliates. When there is an affiliate associated with the _sourceAccount address, the fingerprint of the affiliate’s address is compared against the fingerprint passed as an argument. If both fingerprints match, then the affiliate’s address is set to zero. As a result, no fees are paid to the affiliate nor the source account (who is expecting to receive 20% of the affiliate’s fees). These fees are taken from the total amount of market creator fees.
When a user trading in a market has been referred by the market creator, then the market creator should receive the corresponding affiliate fees from the user’s trading operations. In this case, to avoid paying 20% of the affiliate fees to the user, the market creator can execute front-running transactions that will cause the following trading operation to pay zero fees to affiliates. Specifically, the market creator should:
setFingerprint function, aiming to change her account’s fingerprint to the one used by the user.If the front-run succeeds, then the stored fingerprint and the one submitted by the user should match. As a consequence, the execution of recordMarketCreatorFees will not pay the 20% of the affiliate fees to the user (because the execution would never jump into the if block in lines 347 to 355). Note that this attack should only be profitable for a market creator when the amount to be paid to the user (i.e., 20% of the affiliate fees) is greater than the cost of submitting the front-running transaction to change the fingerprint.
This issue is related to another issue already anticipated by Augur: the fingerprint scheme is intended to prevent users from registering a different address that they control as their affiliate, but it can be easily bypassed by simply changing the fingerprint to a unique value per address. Any solution that can effectively prevent Sybil attacks (where individual users can gain an advantage by controlling multiple accounts) would typically involve identity-based schemes, such as the KYC process intended to be used with the AffiliateValidator contract.
Since the fingerprint protection can be easily bypassed and it also introduces the vulnerability described here, consider removing the fingerprint mechanism entirely.
Alternatively, one plausible course of action to be analyzed is restricting the amount of times an account can modify its fingerprint in the Affiliates contract. More importantly, if there are no reasons why an account should ever update its fingerprint, consider reverting all calls to the setFingerprint function if the caller has already registered a fingerprint in the contract.
Update: Augur has decided to accept this risk because the market creator already chooses an affiliate validation contract, as well as the market creator fees that are distributed. While we understand Augur’s stance, we keep our reservations on whether this risk should be accepted, considering that it allows market creators to avoid paying fees owed to individual traders.
The addKey function of the AffiliateValidator contract is used to associate a key with a user account. The key is expected to be derived from the user’s KYC information. The function first confirms that a given key-salt pair was signed by an operator and that the salt has not been used already. Then it associates the key with the sender’s account.
However, the signature is not cryptographically tied to the caller’s address (i.e., the msg.sender address). This means that an attacker can obtain the signature before it is confirmed (e.g., by reading it in a node’s public pool of pending transactions), and later call the addKey function to associate the stolen key with the attacker’s account. This will allow the attacker to register a valid key without going through the mandatory KYC process. Furthermore, it will also prevent the victim from using the signature.
Consider modifying the addKey function of the AffiliateValidator contract to include the user’s address within the key hash, so as to ensure the signature can only be redeemed by the intended account.
Update: Fixed in PR #5564. The user’s address is included within the key hash.
The Market contract has a preemptiveDisputeCrowdsourcer variable that allows users to pool funds in support of the tentative winning outcome in anticipation of a dispute. If a dispute is successful, it becomes the new tentative winning outcome and the preemptiveDisputeCrowdsourcer, if it is non-zero, becomes a counter-dispute contract in favor of the previous outcome.
However, it is possible for the preemptiveDisputeCrowdsourcer to (inconsistently) support an outcome that is not the current tentative winning outcome. In particular, when a different market in Augur undergoes a fork and disavowCrowdsourcers is called on the non-forking market, the dispute chain is reset to the initial reporter but the preemptiveDisputeCrowdsourcer is not reset. While the initial reporter becomes the tentative winning outcome, it may have a different payout distribution hash to the preemptiveDisputeCrowdsourcer.
In this scenario, should a user call the contributeToTentative function with the correct payout numerators corresponding to the tentative winning outcome, the function will eventually call internalContribute with overload set to true. As a consequence, the internal call to getOrCreateDisputeCrowdsourcer will retrieve the (inconsistent) preemptiveDisputeCrowdsourcer, and the user funds will be incorrectly added to this outcome.
Consider resetting the preemptiveDisputeCrowdsourcer in the disavowCrowdsourcers function.
Update: Fixed in PR #5564. The preemptiveDisputeCrowdsourcer state variable is now is reset in the disavowCrowdsourcers function.
The addKey function of the AffiliateValidator contract is used to associate a key with a user account. The key is expected to be derived from the user’s KYC information. The function first confirms that a given key-salt pair was signed by an operator and that the salt has not been used already. Then it associates the key with the sender’s account.
However, should the signer be a registered operator on multiple instances of the same AffiliateValidator contract, any user can take a signature intended for one contract and apply it to any other. As a consequence, any user would be able to obtain valid keys without going through the mandatory KYC process.
Depending on the expected scenarios where multiple AffiliateValidator contracts share operators, it may be acceptable for the same user to reuse a key (assuming the reported “[H03] Affiliate keys can be stolen” issue is first addressed). Otherwise, consider including the contract’s address within the key hash to ensure the signature can only be used with the expected AffiliateValidator contract.
Update: Fixed in PR #5564. The AffiliateValidator contract address is included within the key hash.
The withdrawAffiliateFees function of the Market contract allows anyone to trigger a withdrawal of all fees an affiliate has earned. While the corresponding affiliate fees are correctly set to zero, the total amount of affiliate fees (tracked in the totalAffiliateFeesAttoCash state variable) is never decreased. As a result, all Augur markets can end up in an inconsistent state where the sum of all entries in the affiliateFeesAttoCash mapping does not exactly add up to totalAffiliateFeesAttoCash.
This issue does not pose a security risk for the Augur protocol. Still, consider always decreasing the totalAffiliateFeesAttoCash after a withdrawal of affiliate fees to ensure consistency and avoid breaking a relevant invariant of the Market contract.
Update: Fixed in PR #5564. This is actually the intended behavior so totalAffiliateFeesAttoCash has been renamed to totalPreFinalizationAffiliateFeesAttoCash for clarity.
The public getFinalizationReward function of the WarpSync contract is intended to retrieve the finalization reward of a given market. Internally, it calls the getRepReward private function passing the end time of the market’s dispute window as an argument.
If getFinalizationReward is called when the market’s dispute window is not yet over (i.e., when block.timestamp is lower than _market.getDisputeWindow().getEndTime()), then the unsafe arithmetic operation in line 77 will inevitably underflow. As a result, the returned amount of REP representing the finalization reward will be erroneous.
Note that this issue does not introduce a security risk on its own. The only time the getFinalizationReward function is used internally by Augur is at the recordMarketFinalized function, when the market’s dispute window should already be over. At this point, no underflows should occur when calculating the reward. Additionally, the getCreationReward function is not affected by the unsafe arithmetic operation mentioned in the previous paragraph, as any known Augur universe is expected to have a lower creation time than the current block.timestamp.
Yet, to prevent unexpected behaviors when querying the getFinalizationReward function from off-chain clients (or other functions in future changes to the code base), consider reverting the transaction when an underflow occurs. Alternatively, depending on Augur’s use cases for this getter function, consider returning zero as the REP reward when the given market’s dispute window is not yet over.
Update: Fixed in PR #5564. The WarpSync contract now uses SafeMathUint256 in all calculations.
Several functions in the code base do not emit relevant events to log their execution. In particular:
GnosisSafeRegistry contract, external functions register and deRegister do not emit events when executed.AffiliateValidator contract, ownership transfer does not emit an event. The onTransferOwnership function has been declared but its body is empty. For consistency, consider following the pattern used in the onTransferOwnership function of the Market contract.WarpSync contract, the recordMarketFinalized function should emit an event whenever the data for the current universe is updated.Market contract, the transferRepBondOwnership function should emit an event whenever the address in repBondOwner is successfully updated.Consider defining and emitting events whenever sensitive changes occur. This would allow effective notifications to off-chain clients about important modifications in the system.
Update: Partially fixed in PR #5577. The events have been added and logged except for the onTransferOwnership function of the AffiliateValidator contract, since the additional complexity would not be worth the gain.
Contracts in the GnosisSafe.sol file do not entirely match any of the known implementations available in Gnosis’s repository. Augur’s GnosisSafe contract claims to correspond to version 1.1.0, yet several differences with the official 1.1.0 version were detected. Moreover, while the GnosisSafe.sol file claims to have been verified in Etherscan on 2019-03-28, that is the date when the 1.0.0 version of the official Gnosis Safe Multisig wallet was submitted for verification (see code in Etherscan. The address was taken from Gnosis’s v1.0.0 zos.mainnet.json file).
Following we detail, for each contract in GnosisSafe.sol, which version matches (if any):
Enum contract: Augur’s implementation matches v1.0.0, not v1.1.0. See differences between v1.0.0 and v1.1.0.EtherPaymentFallback contract: Augur’s implementation matches v1.1.0.Executor contract: Augur’s implementation matches v1.0.0, not v1.1.0. See differences between v1.0.0 and v1.1.0.SecuredTokenTransfer contract: Augur’s implementation matches v1.0.0, not v1.1.0. See differences between v1.0.0 and v1.1.0SelfAuthorized contract: Augur’s implementation matches v1.1.0.ModuleManager contract: Augur’s implementation matches v1.0.0, not v1.1.0.OwnerManager contract: Augur’s implementation matches v1.1.0.MasterCopy contract: Augur’s implementation matches v1.0.0, not v1.1.0. See differences between v1.0.0 and v1.1.0.Module contract: Augur’s implementation matches v1.1.0.SignatureDecoder contract: Augur’s implementation matches v1.1.0.SafeMath library: Augur’s implementation matches v1.1.0.ISignatureValidatorConstants contract: Augur’s implementation matches v1.1.0.ISignatureValidator contract: Augur’s implementation matches v1.1.0.FallbackManager contract: Augur’s implementation matches v1.1.0.GnosisSafe contract: Augur’s implementation does not fully match any of the known versions. While the VERSION claims to be 1.1.0, the contract’s code does not match the official v1.1.0. The most notable modifications from v1.1.0 are:
A new checkTransaction private function has been defined to replace the code block in lines 122 to 133.
executeTransaction private function was defined to replace the code block in lines 135 to 148.require statement that checked the gas left in the transaction has been commented out.checkSignatures function, a require statement to validate that the threshold state variable (inherited from the OwnerManager contract) was initialized (i.e., greater than zero) has been removed.Note that the GnosisSafe.sol file was left out of the audit’s scope. We only checked for compliance with known implementations, but did not audit Augur’s implementation to verify functionality nor security. Consider either clearly documenting the above mentioned differences to raise awareness, or implementing a known non-modified version of the GnosisSafe wallet. Should Augur choose the latter, consider using the latest GnosisSafe version (1.1.1).
The transferRepBondOwnership function of the Market contract does not prevent the new owner from being the zero address.
In this situation, an initial report by the designated reporter or a fork migration could cause a transfer of REP tokens to the zero address, thus bypassing a restriction imposed by the ERC777 specification.
Consider restricting the REP bond owner to non-zero addresses.
Update: Fixed in commit 53a59728.
When the Warp Sync market is finalized, the recordMarketFinalized function of the WarpSync contract is called to reward the transaction sender and to update the data mapping with the latest values.
However, if the market is migrated to a child universe and then finalized, a consistency check in the notifyMarketFinalized function will inevitably fail, thus never calling recordMarketFinalized. As a result, the finalization of the market is not going to be recorded in the WarpSync contract and the user that initiated the finalization transaction will not be rewarded.
Consider handling the special case where a recurring market changes universe before it is finalized, or clearly documenting this behavior.
Update: Fixed in commit b95d70d7. The WarpSync market can no longer migrate.
The deployProxyWithNonce function of the ProxyFactory contract does not use the msg.sender variable to compute the salt that is later passed to create2. As a consequence, two different callers can effectively deploy a proxy to the same address – potentially opening a race condition scenario should the saltNonce have insufficient entropy.
While this does not pose a security issue for Augur, consider either using msg.sender to calculate the salt or explicitly documenting this behavior in the docstrings of the deployProxyWithNonce function to avoid misuses. For an example of the former, refer to OpenZeppelin’s SDK ProxyFactory contract.
The deployProxyWithNonce function of the ProxyFactory contract does not revert the transaction when the create2 operation fails. Depending on the initializer payload, this may or may not cause the transaction to fail.
Following the “fail early and loudly” principle, consider reverting the transaction when the address returned by create2 is zero. For an example, refer to OpenZeppelin’s SDK ProxyFactory contract.
Update: Fixed in commit 7787dba1.
The docstrings of the Proxy contract’s fallback function state that the function “forwards all transactions”. However, transactions with calldata equal to 0xa619486e00000000000000000000000000000000000000000000000000000000 (i.e., calling a masterCopy() getter) will not be forwarded to the masterCopy address.
Consider modifying the function’s docstrings to correctly describe its exact behavior.
The Proxy and ProxyFactory contracts include assembly blocks. See lines 25 to 37, 58 to 60, 88 to 90 and 104 to 106 in ProxyFactory.sol. Taking the first case as example, on top of the delegatecall-related logic found in typical proxy contracts, the assembly block intends to build a public getter for the first slot of the contract’s storage (i.e., the masterCopy internal state variable).
As this is a low-level language that is harder to parse by regular users, consider including extensive documentation regarding the rationale behind its use, clearly explaining what every single assembly instruction does. Clear documentation should make it easier for users to trust the code, for reviewers to verify it, and for developers to build on top of it or update it. As an example, refer to OpenZeppelin’s SDK Proxy contract.
Note that the use of assembly discards several important safety features of Solidity, which may render the code more error-prone. While no issues were identified in the audited version, consider implementing thorough tests to cover all potential use cases of these contracts to programmatically ensure they behave as expected, which should also help prevent introducing regression bugs in future modifications to the code base.
The transferOwnership function of the Ownable contract returns a boolean indicating if the transfer succeeds. However, it returns true in all cases, even when the _newOwner parameter is zero and the contract’s owner is not updated. Consider returning false in this scenario.
Update: Fixed in commit 550a6511.
In the Affiliates contract, there is a require statement without an error message.
In the Market contract, most of the require statements do not include error messages. The only statements with error messages can be seen in lines 135 and 210.
Consider always including specific and informative error messages in all require statements.
The GnosisSafeRegistry, Affiliates, AffiliateValidator and WarpSync contracts completely lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to assess not only security, but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned and the events emitted.
Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Fixed in commit 6e2c2558. The documentation for these contracts has significantly improved.
The doInitialReport function of the WarpSync contract is supposed to return a boolean but does not explicitly return a value. Consider including a return statement in the function.
Update: Fixed in commit 8ca7638e.
The AffiliateValidator contract defines a mapping of operators. Operators are registered by the contract owner and are identified with their Ethereum address. The main purpose of an operator is to sign approved keys for users that go through a Know Your Customer (KYC) process, which will validate users in the affiliate system for any Augur market that specifies the corresponding AffiliateValidator contract.
Even though operators are expected to be fully trusted entities, their powers must be understood in depth to be aware of misbehaviors. For this reason, following we highlight three potentially unexpected actions that can be carried out by rogue operators.
addKey function in a node’s pool of pending transactions, they can front-run it with a new transaction using the same salt as the victim’s transaction. If the rogue operator effectively front-runs, then the salt will be registered as used and the second transaction using that same salt will fail. The rogue operator can do this indefinitely until being removed by the owner of the AffiliateValidator contract. This would prevent any other new key from being added, thus causing a Denial of Service on the targeted contract.validateReference function would be reverted.validateReference function would be reverted.Consider explicitly documenting the role of operators in Augur’s affiliate system, highlighting what is the set of expected actions that they can carry out in the protocol.
The keys mapping of the AffiliateValidator contract stores a user-specific key (a bytes32 value), computed and signed by a trusted operator. The Augur team indicated that the key might be a hash of the user’s Know Your Customer (KYC) information, such as phone number and IP address. Different operators with the same information will produce the same user-specific key.
It should be noted that in this context the hash provides minimal confidentiality. This is because the number of possible inputs to produce the hash is likely small enough to be exhaustible. Since the key will be publicly available and associated directly with the user’s Ethereum address, an attacker can simply try all possible inputs until the correct hash is found, thereby revealing the KYC information. Typically, a large private random number is included when hashing low-entropy inputs to avoid this issue. Yet this would undermine the goal of having the same KYC information produce the same key.
The KYC procedure is out of scope for this audit, and revealing KYC information does not introduce a security vulnerability in the Augur protocol itself. Nevertheless, we highlight this issue as it may have not been considered and we understand the Augur team should be aware of the risk.
The AffiliateValidator contract accepts ECDSA signatures over messages that prepend the appropriate magic constant. However, it does not restrict the possible value of s and v more than the native ecrecover operation. To avoid signature malleability, it is common practice to restrict s to the lower range and restrict v to either 27 or 28.
Note that the AffiliateValidator contract is not vulnerable to signature malleability (within a single contract instance) because the salt used in the signature must be unique. Nevertheless, to favor consistency, consider implementing the additional restrictions. Consider using OpenZeppelin’s ECDSA library as a reference implementation.
The getAndValidateReferrer function of the Affiliates contract intends to return the zero address when the _referrer address is zero, or when it matches the passed _account address. However, this last scenario is impossible to reach with a non-zero _referrer address. This stems from the fact that the setReferrer function (the only entry point to modify the referrals mapping) already ensures that the key and value cannot be the same.
To favor readability and avoid unnecessary validations, consider removing the unfulfillable condition.
To favor consistency and explicitness, consider modifying the Affiliates contract so that it inherits from the IAffiliates contract.
Lines 151 and 215 of the ERC1155 contract both explicitly compare a boolean value to true. This is a redundant operation because the result will be equivalent to the boolean value itself. Consider removing the redundant comparison.
The isApprovedForAll function of the ERC1155 contract returns true if the queried operator is the token contract itself. Although the contract can update balances at will, referring to it as an operator for a particular account may be confusing, particularly since it introduces an inconsistency between the isApprovedForAll function and the ApprovalForAll events. This behavior is also inconsistent with the ERC1155 reference implementation.
Consider returning false when the queried operator is the token contract itself.
The _burn and _burnBatch functions of the ERC1155 contract both accept a doAcceptanceCheck flag that indicates whether they should call the onERC1155Received hook (or onERC1155BatchReceived) on the destination. However, the flag has no effect in these functions as the destination in both is the zero address.
To favor simplicity, consider removing the doAcceptanceCheck flag, and related logic, from the internal burn functions of the ERC1155 contract.
The notifyMarketFinalized function of the WarpSync contract only allows creation of a new recurring market if the universe is not undergoing a fork. However, this condition is already checked during the call to the recordMarketFinalized function. Internally, recordMarketFinalized calls awardRep, which in turn calls the mintForWarpSync function of the ReputationToken contract. Finally, the execution reaches the updateForkValues function of the Universe contract. This function first validates that the current universe is not forking before continuing execution.
Similarly, the migrateThroughOneFork function of the Market contract ensures the market is not finalized, but this condition is checked in the following call to disavowCrowdsourcers
To favor simplicity and reduce gas costs, consider removing the redundant validations.
The GnosisSafeRegistry contract implements two different functions for registering new Gnosis Safe Multisig wallets: register and callRegister. However, by calling register instead of callRegister, it is possible to register wallets that:
Augur contractCreateOrder contractFillOrder contractAffiliates contractConsider either providing a single function for registering wallets in the GnosisSafeRegistry contract, or explicitly documenting the described inconsistent behavior between the register and callRegister functions, highlighting under which circumstances each function is expected to be called.
GnosisSafeRegistry contract, the getSafe function should be renamed to getCurrentSafe, getActiveSafe or similar.WarpSync contract:MARKET_LENGTH should be renamed to MARKET_DURATIONData and state variable data should have more self-explanatory names.The following state variables are implicitly using the default visibility:
AffiliateValidator contract: keys, operators and usedSalts.Market contract: affiliateValidator and affiliates.To favor readability, consider explicitly declaring the visibility of all state variables.
Named return variables are used inconsistently. For example, while the ProxyFactory contract names its return variables, the Affiliates contract does not. Consider removing all named return variables, explicitly declaring them as local variables, and adding the necessary return statements where appropriate. This should improve both explicitness and readability of the project.
In line 344 of Market.sol, considering replacing address(0) with the NULL_ADDRESS constant to favor consistency.
The following imports are not used:
IWarpSync.sol file unnecessarily imports the IUniverse contract.Market.sol file unnecessarily imports the IERC1155 contract.Consider removing the unnecessary imports.
On the other hand, the following contracts rely on indirect imports:
GnosisSafeRegistry.sol uses the IAugur.sol imports to obtain the IERC20 contract.WarpSync.sol uses the IMarket.sol imports to obtain the IAugur contract.Consider importing these contracts explicitly.
ProxyFactory contract’s docstrings: contact should be contract. There are five instances of this typo in the contract’s file.Affiliates.sol: their fingerprint should say its fingerprint and naievly should say naively.AffiliateValidator.sol: their key should say its key.ERC1155 contract: all instances of _supplys should be _supplies. Moreover, in line 188: the comment should say onERC1155BatchReceived instead of onERC1155Received.Market.sol: tenative should be tentative.Market.sol: occured should be occurred.Market.sol: finalzied should be finalized and timestmap should be timestampMarket.sol: has should be hash.WarpSync.sol: occuring should be occurring.To favor explicitness, all instances of uint should be declared as uint256. See for example lines 280 and 322 of ERC1155.sol.
To increase the code’s readability, consider always explicitly casting all instances of this using address(this). Some examples of this issue can be found in lines 763, 766 and 785 of Market.sol.
The migrateThroughOneFork function of the Market contract defines a local variable _numOutcomes that is never used. Consider removing the definition.
1 critical and 4 high severity issues were found, all of them fixed and reviewed. Several recommendations were made to improve the project’s overall quality and reduce its attack surface.