We audited the aukilabs/auki-contracts repository at commit 841aa7d.
In scope were the following files:
src
├── AukiToken.sol
├── BurnContract.sol
├── RewardLiquidityPoolContract.sol
├── StakingContract.sol
└── UUPSProxy.sol
Auki Labs is a company creating products that target the spatial computing space. Specifically, they provide solutions powered by their posemesh system, which has a decentralized infrastructure and leverages blockchain technology to incentivize both developers and operators. The AukiToken, an ERC-20-compliant token, is a utility token used for staking as well as burning. Developers are required to burn these tokens in order to generate posemesh credits. In addition, these tokens are required to be staked in order to maintain a reputation and can be slashed for malicious behavior. When tokens are burned, they are transferred into a liquidity pool contract where rewards for participating in the network are later distributed amongst the users.
The posemesh is currently a centralized system. While the team desires to gradually decentralize the project, for the earlier iterations, many of the components will be governed by Auki Labs (both on-chain and off-chain).
A co-managed MPC wallet that requires Auki MPC signatures along with the custodian signature (after fulfilling KYC requirements for the custodian) is responsible for deploying these contracts. As a result, this wallet is minted 1e28 Auki tokens when the AukiToken contract is deployed. Furthermore, it receives the DEFAULT_ADMIN_ROLE role for the AukiToken, BurnContract, RewardLiquidityPoolContract, and StakingContract contracts. Since it has the default admin role, this wallet can grant itself (or any other address) any of the roles in these contracts, as well as revoke the roles. As such, this wallet has the ability to upgrade the implementation for any of the upgradeable contracts in this repository.
Another MPC wallet that only requires Auki MPC signatures and approval without KYC is intended to have the PAUSER_ROLE, SLASHER_ROLE, REWARD_OPERATOR_ROLE, and CONFIG_ROLE roles. Although not specified explicitly in the code, the fact that this wallet will have the above roles was communicated to us through our messages with the Auki Labs team.
While this wallet cannot upgrade the code, nor grant or revoke roles, it is nonetheless an extremely privileged role. This MPC wallet can pause the AukiToken contract, which would prevent any mints, burns, or transfers, as well as any staking, slashing, or withdrawing. This wallet also can slash any stake at will, reward any address any amount up to the total amount of funds available, as well as the set the amount needed for a staker and the time before any staker can withdraw their funds.
The BurnContract is granted the MINTER_ROLE and BURNER_ROLE roles, and the StakingContract is granted the BURNER_ROLE role.
The AukiToken contract can be paused and unpaused by an address that has been granted the PAUSER_ROLE. Through the use of the whenNotPaused modifier on the _beforeTokenTransfer function, when the AukiToken contract is paused, no tokens can be transferred, minted, or burned. However, this does not prevent approvals, permits, and/or any other allowance changes from taking place while the contract is paused.
According to the Auki Labs team, "the expected behavior when [Auki] token is paused is to pause all operations.". As such, consider adding the whenNotPaused modifier to all corresponding allowance change functions to prevent this from occurring while the contract is paused.
Update: Resolved in pull request #48 at commit 5c27964.
DEFAULT_ADMIN_ROLEThe AukiToken, BurnContract, RewardLiquidityPoolContract, and StakingContract contracts have access control (or its upgradeable counterpart). The DEFAULT_ADMIN_ROLE role is given to the msg.sender when deploying the contract, which is the co-managed MPC wallet. The admin role for all other roles in this system is the DEFAULT_ADMIN_ROLE role, since it is not explicitly assigned in the code. Since this role has special privileges associated with it, for example the ability to grant and revoke roles, further security measure should be taken.
According to the comments in OpenZeppelin's AccessControl.sol file as well as the official documentation, the recommendation is to use AccessControl in conjunction with AccessControlDefaultAdminRules. This contract implements the following risk mitigations on top of AccessControl:
DEFAULT_ADMIN_ROLE role since deployment until it is potentially renounced.DEFAULT_ADMIN_ROLE role to another account.DEFAULT_ADMIN_ROLE role.Consider adding AccessControlDefaultAdminRules on top of the existing AccessControl in order to safely manage the DEFAULT_ADMIN_ROLE role.
Update: Resolved in pull request #48 at commit ce493df.
isProxied From ConstructorsThere are instances of test code across several upgradeable contract constructors:
The isProxied parameter is used by the development team in order to use the Blacksmith tool. Blacksmith currently does not support interacting with a contract that uses a proxy upgrade pattern. However, deploying these contracts without the use of a proxy in production can create the risk of an attacker front-running the initialize function. This would lead to a redeployment of the contracts and wastage of gas.
Consider removing the isProxied parameter from the constructors in a production-ready codebase.
Update: Resolved in pull request #48 at commit 3597610.
Throughout the codebase, the constructors and initializers do not emit events after initializing sensitive variables in the system. Although in some cases, when those variables are updated using setter functions, an event is emitted:
initialize function of BurnContract, both storage variables token and recipient are set without emitting any events.constructor of RewardLiquidityPoolContract, the storage variable token is set without emitting an event.initialize function of StakingContract, the storage variable stakingToken is set without emitting an event. Furthermore, the storage variables thawingPeriod and stakeAmount are set without emitting the UpdatedThawingPeriod and UpdatedStakeAmount events, respectively.Consider emitting events when updating state variables to more easily enable off-chain monitoring for the protocol.
Update: Resolved in pull request #48 at commit edd27f0.
Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of a mapping's purpose.
Throughout the codebase, there are multiple mappings without named parameters:
stakes state variable in the StakingContract contract_rewards state variable in the RewardLiquidityPoolContract contractConsider adding named parameters to the mappings in order to improve the readability and maintainability of the codebase.
Update: Resolved in pull request #48 at commit 25f7718.
In StakingContract, there are multiple variables that are unnecessarily casted:
stakingToken variable in the slash function_stakingToken variable in the initialize functionTo improve the overall clarity, intent, and readability of the codebase, consider removing these unnecessary casts.
Update: Resolved in pull request #48 at commit 437191d.
Throughout the codebase, the current format for importing other Solidity files is from the lib folder of the repository. For example, the OpenZeppelin contract imports take the form of import {SomeContract} from "lib/openzeppelin-contracts/*". While this is suitable for developers working only in Foundry, other developers who are using this contract (particularly those who are not using Foundry as their smart contract development framework) may run into compilation errors when trying to integrate.
For ease of consumption, particularly for other development frameworks such as Hardhat and Remix, consider changing the import format to import {SomeContract} from "@openzeppelin/contracts/..." as described in the OpenZeppelin documentation. This pattern can similarly be applied for other imports outside the OpenZeppelin library. In conjunction with this, a remappings.txt file can be added in the home directory to map imports to their Foundry specific paths.
Consider changing the import format as suggested above and adding a remappings.txt. This will help improve the developer experience for developers working in different development frameworks.
Update: Resolved in pull request #48 at commit 496066f.
Potential gas cost improvements were found throughout the codebase:
require statement on token transfers. The AukiToken contract inherits from the OpenZeppelin libraries which already revert with an error message if a transfer fails.
for loops that are using i++ can be optimized by upgrading to Solidity version 0.8.22 where the compiler will automatically generate an unchecked block to save gas.
Stake struct are being individually updated across several functions in StakingContract.sol. The struct can be saved into a local storage variable that can then be used and updated as needed. This applies to the following functions:
for loops and require statements in RewardLiquidityPoolContract.sol. Specifically, the recipients.length in the following functions could be stored in a temporary variable:
calldata keyword can be used instead of memory for dynamic array function parameters when the function is being called externally and the parameter is not manipulated within the function itself. This leaves the data inside of the calldata, rather than copying it to memory, which saves gas. The following examples were found in the codebase:
Consider applying these gas optimizations.
Update: Partially resolved in pull request #48 at commit 265a519. The local variables were added, the calldata keyword was used in the appropriate locations, and all the extraneous require statements were removed. The for loops were left unoptimized. The Auki Labs team stated:
We are constrained to use 0.8.19.
In RewardLiquidityPoolContract, the balance function does not refer to the token balance of the contract, but rather is the token balance with the totalPendingReward subtracted from it. Consider updating the name of this function to nonPendingBalance for clarity and to prevent name-clashing with the commonly used name balance.
Consider applying the above naming suggestion to improve the consistency and readability of the codebase.
Update: Resolved in pull request #48 at commit e42401c.
RewardLiquidityPoolContract Is Not UpgradeableCurrently, RewardLiquidityPoolContract is a non-upgradeable contract. Therefore, in the event that there is a need to update its code, the storage variables will not persist once a new version is deployed (i.e., the _rewards mapping will be reset). To preserve the values of the storage variables even after an upgrade, consider making RewardLiquidityPoolContract upgradeable.
Update: Resolved in pull request #48 at commit ce493df.
virtual KeywordIn StakingContract, the _stake and _withdraw functions have the virtual keyword. This indicates to a reader that these functions are intended to be overridden. However, the Auki Labs team has stated that they "do not plan to inherit from these contracts".
Consider removing the virtual keyword to show clear intent to other developers and end users who are reading the code.
Update: Resolved in pull request #48 at commit 768c7eb.
immutable Keyword for Unchanging VariablesIn BurnContract, RewardLiquidityPoolContract, and StakingContract, the variable representing the AukiToken is a state variable. However, it cannot be changed by any function in the contract.
Consider adding the immutable keyword to this variable along with moving the variable initialization into the constructor. This will help save gas as well as improve code readability.
Update: Resolved in pull request #48 at commit d4c88ad.
.env Does Not Follow Best PracticeThe .env file that has been pushed to Github contains a PRIVATE_KEY used for Anvil testing. Although there are no real funds on this network, it is best to have sensitive information stored offline and only an example stored in an .env.example file. Doing this will help prevent accidentally pushing a mainnet private key to Github, potentially resulting in the theft of funds.
Consider removing sensitive information from the .env file on Github and only leaving an example in an .env.example file.
Update: Acknowledged, not resolved. The Auki Labs team stated:
The leaked private key is the one provided by the Anvil tool. It is already a leaked key and is used by everyone using Anvil from the Foundry toolchain. As such, we may keep it to ease the developer workflow.
Auki Labs aim to utilize blockchain technology to incentivize developers and operators through the AukiToken. The token is burned for posemesh credits, staked for reputation, and can be slashed for malicious behavior, with the rewards being distributed from a liquidity pool contract.
This was the first audit that the OpenZeppelin team has performed for Auki Labs. In the report, we uncovered a couple of medium-severity issues related to the integration of the OpenZeppelin libraries. We would like to thank the Auki Labs team for their quick and thorough responses which greatly helped our process.
The codebase was well-tested with nearly 100% coverage. While we understand that this project is still in its infancy and therefore in a centralized state, we hope that over time, it will slowly decentralize into a protocol with less trust assumptions and privileged roles.