Security Audit
March 27, 2023
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for Treasure DAO's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from January 9, 2022 to February 27, 2023.
The purpose of this audit is to review the source code of certain Treasure DAO Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.
Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.
The following is an aggregation of issues found by the Macro Audit team:
Severity | Count | Acknowledged | Won't Do | Addressed |
---|---|---|---|---|
High | 3 | - | - | 3 |
Medium | 7 | 1 | 1 | 5 |
Low | 9 | 1 | - | 8 |
Code Quality | 13 | 5 | - | 8 |
Treasure DAO was quick to respond to these issues.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
1a57363c24209721543a3271ec255ac2b9bff214
Specifically, we audited the following contracts within this repository:
Source Code | SHA256 |
---|---|
src/Rewards/StakingContractMainnet.sol |
|
src/Rewards/libraries/FullMath.sol |
|
src/Rewards/libraries/PackedUint144.sol |
|
src/Router/IMagicSwapV2Router.sol |
|
src/Router/MagicSwapV2Router.sol |
|
src/UniswapV2/core/UniswapV2ERC20.sol |
|
src/UniswapV2/core/UniswapV2Factory.sol |
|
src/UniswapV2/core/UniswapV2Pair.sol |
|
src/UniswapV2/core/interfaces/IUniswapV2Callee.sol |
|
src/UniswapV2/core/interfaces/IUniswapV2ERC20.sol |
|
src/UniswapV2/core/interfaces/IUniswapV2Factory.sol |
|
src/UniswapV2/core/interfaces/IUniswapV2Pair.sol |
|
src/UniswapV2/core/libraries/Oracle.sol |
|
src/UniswapV2/core/libraries/SafeMath.sol |
|
src/UniswapV2/core/libraries/UQ112x112.sol |
|
src/UniswapV2/core/libraries/UniswapV2Math.sol |
|
src/UniswapV2/libraries/Babylonian.sol |
|
src/UniswapV2/libraries/BitMath.sol |
|
src/UniswapV2/libraries/FixedPoint.sol |
|
src/UniswapV2/libraries/FullMath.sol |
|
src/UniswapV2/libraries/TransferHelper.sol |
|
src/UniswapV2/periphery/UniswapV2Router02.sol |
|
src/UniswapV2/periphery/interfaces/IERC20.sol |
|
src/UniswapV2/periphery/interfaces/IUniswapV2Router01.sol |
|
src/UniswapV2/periphery/interfaces/IUniswapV2Router02.sol |
|
src/UniswapV2/periphery/interfaces/IWETH.sol |
|
src/UniswapV2/periphery/libraries/OracleLibrary.sol |
|
src/UniswapV2/periphery/libraries/UniswapV2Library.sol |
|
src/Vault/INftVault.sol |
|
src/Vault/INftVaultFactory.sol |
|
src/Vault/NftVault.sol |
|
src/Vault/NftVaultFactory.sol |
|
Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.
Click on an issue to jump to it, or scroll down to see them all.
MAX_FEE
defaultFees
is set it is not possible to reduce lpFee
or protocolFee
to 0 for individual pair
setRoyaltiesFee()
defaultFees
MagicSwapV2Router:addLiquidityNFT()
can be denied of service
swapLeftover()
function
override
keyword to the interface functions
We quantify issues in three parts:
This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:
Severity | Description |
---|---|
(C-x) Critical |
We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost. |
(H-x) High |
We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec. |
(M-x) Medium |
We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner. |
(L-x) Low |
The risk is small, unlikely, or may not relevant to the project in a meaningful way. Whether or not the project wants to develop a fix is up to the goals and needs of the project. |
(Q-x) Code Quality |
The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design. |
(I-x) Informational |
Warnings and things to keep in mind when operating the protocol. No immediate action required. |
(G-x) Gas Optimizations |
The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it. |
Whenever an NFT token is deposited to Vault, ONE
amount of ERC20 Vault tokens is minted to the depositor. To be able to utilize corresponding ERC20 Vault tokens, corresponding UniswapV2Pair is created and initialized. With the call to mint()
function this pair contract is configured with initial liquidity. During initial liquidity provision, MINIMUM_LIQUIDITY
of LP tokens are permanently locked. Due to this, a small amount of LP tokens and consequently ERC20 Vault tokens will become permanently inaccessible.
Due to some amount of ERC20 Vault tokens becoming permanently inaccessible, it will not be possible to withdraw all NFTs from the Vault even in case of obtaining all ERC20 Vault tokens on the market. In other words, one NFT cannot be withdrawn. As a result, some amount of ERC20 Vault tokens (smaller than ONE
) will be unbacked by the corresponding NFT token. The impact of this issue varies depending on how many NFT tokens a particular Vault is handling.
Remediations to consider:
Additional commit 5da6887caf29825d52604ea63606c4faafda1bf1
The intention for soulbound vaults is to tokenize items that are not meant for sale.
The design of the solution is not to let AMM and its router be the recipient of a soulbound vault tokens’ transfer.
The implementation restricts transfer recipients to an EOA / a soulbound vault / an allowed list of contracts.
// NftVault.sol
function _beforeTokenTransfer(
address /*from*/,
address to,
uint256 /*amount*/
) internal view override {
/// @dev Soulbound Vault ERC20 token can be transfered to any EOA, this Vault or `allowedContracts`
if (
isSoulbound
&& to != address(this)
&& Address.isContract(to)
&& !allowedContracts[to]
) revert SoulboundTransferDisallowed();
}
However, an attacker can bypass the recipient restriction by using the fact that a contract is deemed to be an EOA before its constructor finishes.
// Address.sol
function isContract(address account) internal view returns (bool) {
// This method relies on extcodesize/address.code.length, which returns 0
// for contracts in construction, since the code is only stored at the end
// of the constructor execution.
return account.code.length > 0;
}
Specifically, an attacker trying to bypass the recipient restriction:
vault.approve(predetermined-contract-addr, type(uint256).max)
Note that by restricting to EOAs and allow list of contracts, smart contract wallets will need to be approved one by one.
Remediations to consider
Vault was split into permissioned and permissionless. Permissioned vault kept original code and is made out of scope of this audit. Further work will be done in the future to secure permissioned functionality.
In the modified UniswapV2Pair
contract, _takeFees()
is called within swap()
function to perform calculation and distribution of protocol and royalty fee to corresponding recipients. The current implementation assumes that only one of the input arguments amount0In
or amount1In
is set. It optimistically sets the value of the amount
(base amount on which fees will be applied) to amount0In
and overrides it with amount1In
if this argument is greater than 0. Also, fees will be paid in token0
unless amount1In
is set. If amount1In
is greater than 0, fees will be paid in token1
.
However, the assumption made in the current implementation is incorrect. Any malicious 3rd party may send directly to the contract a minuscule token1
amount. This will cause swaps from token0
to token1
to take a particular execution path in _takeFees()
that will result in an incorrect calculation and distribution of fees to expected recipients (protocol and royalty fee recipients). Consequently, fees that were appropriately calculated in swap()
function, but not properly distributed in _takeFees()
, will accrue in the contract and improve the position of liquidity providers.
Remediations to Consider:
amount0In
and amount1In
if they are set. Note that the trivial solution of generating an error when both of these arguments are set can result in a griefing attack vector.In the MagicSwapV2Router
contract, the _withdrawVault()
exchanges Vault ERC20 tokens for NFTs in interaction with the underlying Vault. This function transfers amountToBurn
tokens to the underlying vault. This amount is based on _amount
input. The NFT tokens to retrieve back are defined by _collection
and _tokenId
inputs.
IERC20(address(_vault)).transferFrom(_from, address(_vault), amountToBurn);
amountBurned = _vault.withdrawBatch(_to, _collection, _tokenId, _amount);
Processing happens in the withdrawBatch()
function defined in the NftVault
contract.
function withdrawBatch(
address _to,
address[] memory _collection,
uint256[] memory _tokenId,
uint256[] memory _amount
) external returns (uint256 amountBurned) {
for (uint256 i = 0; i < _collection.length; i++) {
amountBurned += withdraw(_to, _collection[i], _tokenId[i], _amount[i]);
}
}
User may call by accident withdrawVault()
function with _collection
input of a smaller length than the _amount
input. This leads to user ERC20 tokens being transferred to the underlying vault but none (when _collection
is an empty array) or fewer than the expected number of NFTs (when _collection
is an array of smaller length than the _amount
) sent back to the user. Note that withdrawBatch()
will successfully process empty _collection
without generating an error even if _amount
is not empty.
Remediations to Consider:
_withdrawVault()
function that the amount of tokens transferred to the Vault matches the amount burned amountToBurn == amountBurned
MAX_FEE
With UniswapV2Factory
, there are three setters for three fees respectively:
Each setter checks to make sure the fee is set below MAX_FEE
. However, their sum can be set above MAX_FEE
deviating from the intention. In this case, in _getFees()
view function custom logic applies fee caps and prioritizes charging different fee types in a particular order. This can lead to some fees not being applied at all and as a result some participants not receiving their expected revenue share.
Remediations to consider
Consider having 1 setter for all fees and fee beneficiaries to avoid entering into an invalid state. This 1 setter can be reused to set default fees to shorten code. See setFees
and _setFees
in Proof of Concept - UniswapV2Factory.
Unexpected fee rates are well documented and only occur when admin sets too high values which should not happen in the first place. It’s much easier to manage the protocol when a single fee can be changed instead of multiple fees and that was the goal.
In the MagicSwapV2Router
many functions rely on underlying withdrawBatch()
in the NftVault
contract for withdrawing NFTs stored in the Vault.
Some of these functions are withdrawVault()
, removeLiquidityNFT()
, removeLiquidityNFTETH()
, swapTokensForNft()
, swapETHForNft()
, swapNftForNft()
.
Each of these functions accepts three array inputs that define the amounts and identities of particular NFTs to withdraw. If the balance for any of these tokens is less than the requested amount, withdrawBatch()
will revert and the whole transaction will fail.
A malicious attacker may intentionally cause a user’s transaction always to fail if he can sandwich that transaction consistently.
The attacker, after observing the user’s transaction in mempool:
Remediations to consider
withdrawBatch()
in a way in which the request can be partially fulfilled. This does not resolve the issue but it increases the complexity for the attacker to perform the attack since it requires him to have access to proportionally more ERC20 Vault tokens, orWithdrawal must be processed atomically and the only real solution is for user to use flashbots to avoid mempool if such attack happens.
In the NftVaultFactory
anyone can create a permissioned Vault. Permissioned NftVault has different characteristics and an associated risk profile that may not be obvious to end users. For example, both deposit()
and withdraw()
are guarded by onlyAllowed()
modifier that implements access control.
modifier onlyAllowed() {
if (isPermissioned() && !allowedWallets[msg.sender]) {
revert NotAllowed();
}
_;
}
For example, the owner of the permissioned NftVault may allow a particular user to deposit an NFT token. However, it may immediately revoke that user’s privileges so she cannot withdraw NFT if she wants.
Also, when a particular user is allowed to deposit NFT in return, it receives ERC20 Vault tokens which she may sell. The buyer of these tokens might not be aware that by default she will not be allowed to use these tokens to withdraw the NFT which is backing them.
In some circumstances, this functionality may represent a feature. However, any issue of this type may affect the public image of the whole project.
Remediations to consider
onlyAllowed
from withdraw()
), andSoulbound
ERC20 Vault tokens.Vault was split into permissioned and permissionless. Permissioned vault kept original code and is made out of scope of this audit. Further work will be done in the future to secure permissioned functionality.
In MagicSwapV2Router
, which inherits from UniswapV2Router02
, some functions are meant to handle fee-on-transfer tokens, such as swapExactTokensForTokensSupportingFeeOnTransferTokens()
, swapExactETHForTokensSupportingFeeOnTransferTokens()
, and swapExactTokensForETHSupportingFeeOnTransferTokens()
.
However, the custom implementation in MagicSwapV2Router cannot handle these non-standard types of tokens.
Remediations to consider
UniswapV2Router02
, orMagicSwapV2Router
to be able to handle this type of token properlyFee-on-transfer and other non-standard token implementations are not meant to be supported. Corresponding functionality removed.
In UniswapV2Pair
, swap()
function follows Uniswap V2 design and supports flash loans/swaps by optimistically transferring the requested amounts of tokens. After performing an external call, it checks to verify that balances are in the proper state. While flash loans/swaps can represent an additional source of revenue, they also enable malicious attackers to amplify deviations and generate more profitable exploits.
The current project specification does not include flash loans/swaps as a feature. However, its presence increases the complexity of the implementation and introduces extra risks.
Remediations to consider
In UniswapV2Factory
, the contract owner has admin privileges that allow him to set LP, royalty, and protocol fees, as well as corresponding beneficiaries. As a result, the contract owner can change how much and where is revenue distributed from all of the underlying token pairs.
Since these privileged operations represent attractive targets, consider using a more adequate authorization mechanism.
Remediations to consider
Team is going to use a multi-sig wallet for governance.
defaultFees
is set it is not possible to reduce lpFee
or protocolFee
to 0 for individual pair
In the UniswapV2Factory
contract, to override default fees, it is necessary to call setProtocolFee()
and setLpFee()
. To retrieve lpFee
and protocolFee
corresponding functions _getLpFee()
and _getProtocolFee()
are available.
However, due to how these getter functions are implemented, when defaultFees.lpFee
and defaultFees.protocolFee
are set and ≠ 0, it is impossible to override them and set these same fees for particular pair to 0.
Remediations to consider
defaultFees
should be usedsetRoyaltiesFee()
In the UniswapV2Factory
contract, the owner can set royalty-related information for particular pair by calling the setRoyaltiesFee()
function. In this function, a guard is preventing _royaltiesFee
to be set to an invalid value.
function setRoyaltiesFee(address _pair, address _beneficiary, uint256 _royaltiesFee)
external onlyOwner
{
require(_royaltiesFee <= MAX_FEE, 'MagicswapV2: _royaltiesFee > MAX_FEE');
pairFees[_pair].royaltiesBeneficiary = _beneficiary;
pairFees[_pair].royaltiesFee = _royaltiesFee;
}
However, there is no corresponding check for the royaltiesBeneficiary
state variable which can be set to 0 address accidentally. If this happens, swap()
functionality in the underlying pair contract will be bricked since each call to _takeFees()
will revert due to an error raised in the _safeTransfer()
for transferring royalty amount to an invalid royaltiesBeneficiary
.
Remediations to consider
_beneficiary
argument in setRoyaltiesFee()
functionIn the following functions in the UniswapV2Factory contract, there are no checks that provided _pair
is a valid pair contract and one managed by the particular instance of UniswapV2Factory.
setLpFee()
setRoyaltiesFee()
setProtocolFee()
getFeesAndRecipients()
getFees()
getTotalFee()
function setLpFee(address _pair, uint256 _lpFee) external onlyOwner {
require(_lpFee <= MAX_FEE, 'MagicswapV2: _lpFee > MAX_FEE');
pairFees[_pair].lpFee = _lpFee;
}
In setLpFee()
function we can see that pairFees[_pair].lpFee
will be updated even if _pair
is not the correct contract address. Moreover, the transaction will be successfully processed without any error being raised. This may lead to confusion in cases when an incorrect _pair
address is accidentally provided.
Remediations to consider
_pair
address, change function signatures to accept tokenA
and tokenB
addresses, perform a lookup of the corresponding pair
address and revert in case it is not founddefaultFees
In the UniswapV2Factory
contract, the setDefaultFees()
function is called from the constructor to set the initial values of protocolFee
and lpFee
. This function is also callable afterward by the factory owner.
function setDefaultFees(Fees memory _fees) public onlyOwner {
require(_fees.protocolFee <= MAX_FEE, 'MagicswapV2: protocolFee > MAX_FEE');
require(_fees.lpFee <= MAX_FEE, 'MagicswapV2: lpFee > MAX_FEE');
require(_fees.protocolFee + _fees.lpFee <= MAX_FEE, 'MagicswapV2: protocolFee + lpFee > MAX_FEE');
defaultFees = _fees;
}
This function accepts an argument of a Fees
struct type defined in the IUniswapV2Factory
contract. Owner may invoke setDefaultFees()
with royaltiesBeneficiary
and royaltiesFee
values set in provided _fees
argument with intent to update default values for these attributes.
struct Fees {
address royaltiesBeneficiary;
/// @dev in basis point, denominated by 10000
uint256 royaltiesFee;
/// @dev in basis point, denominated by 10000
uint256 protocolFee;
/// @dev in basis point, denominated by 10000
uint256 lpFee;
}
However, even though defaultFees
state variable will be updated, confusingly no corresponding system behavior changes will be observed since these two fields of the defaultFees
variable are not used anywhere.
Remediations to consider
Fees
struct for defaultFees
define a different struct with lpFee
and protocolFee
fields only, ordefaultLpFee
and defaultProtocolFee
insteadIn the UniswapV2Factory
contract, the following setters change important state. However, there is no event emission that would enable off-chain tracking and monitoring systems to react to them.
setLpFee()
setRoyaltiesFee()
setProtocolFee()
setProtocolFeeBeneficiary()
setDefaultFees()
Remediations to consider
In NftVault.sol
, the states allowedWallets
and allowedContracts
, set by the vault owner to (dis)approve the accounts for withdrawal and receiving vault token, respectively.
However, it’s possible the vault owner by mistake approves an account for receiving but not withdrawal so the account cannot redeem NFTs after receiving vault tokens.
This can be a feature in some cases. But this can lead to poor user experience and requires the owner to intervene.
Remediations to consider
allowVaultTokenTransfersTo
when the account is not already approved for deposit/withdrawal.Solved in H-2. Permissions will be removed from the vault and factory making this issue irrelevant.
MagicSwapV2Router:addLiquidityNFT()
can be denied of service
MagicSwapV2Router:addLiquidityNFT()
allows a liquidity provider to provide NFT-tokenB liquidity. It calls into UniswapV2Router02:_addLiquidity()
to get quotes for amountA
and amountB
.
// Inside MagicSwapV2Router:addLiquidityNFT
uint256 amountAMinted = _depositVault(_collection, _tokenId, _amount, _tokenA, address(this));
(amountA, amountB) = _addLiquidity(
address(_tokenA),
_tokenB,
amountAMinted, // amountADesired
_amountBDesired, // amountADesired
amountAMinted, // amountAMin
_amountBMin // amountBMin
);
// Inside UniswapV2Router02:_addLiquidity
// Try fixing amountA to get amountB
uint amountBOptimal = UniswapV2Library.quote(amountADesired, reserveA, reserveB);
if (amountBOptimal <= amountBDesired) {
require(amountBOptimal >= amountBMin, 'UniswapV2Router: INSUFFICIENT_B_AMOUNT');
(amountA, amountB) = (amountADesired, amountBOptimal);
// Try fixing amountB to get amountA
} else {
uint amountAOptimal = UniswapV2Library.quote(amountBDesired, reserveB, reserveA);
// Importantly, amountA is required to be amountAMin
assert(amountAOptimal <= amountADesired);
require(amountAOptimal >= amountAMin, 'UniswapV2Router: INSUFFICIENT_A_AMOUNT');
(amountA, amountB) = (amountAOptimal, amountBDesired);
}
However, because 1) a griefer can control reserveA
and reserveB
and 2) addLiquidityNFT()
implementation requires amountA
in liquidity provision be exactly amountAMinted
, attacker can always make UniswapV2Router02:_addLiquidity()
revert to make MagicSwapV2Router:addLiquidityNFT()
unavailable.
Specifically, a griefer can sandwich:
amountBOptimal
higher so the else
block runs andamountAOptimal
lower than amountAMin
addLiquidityNFT()
revertsNotice also that likelihood of this issue decreases as AMM fees for swaps increase, since the attacker will need to bear costs of manipulating exchange price twice.
Remediations to consider
_amountBDesired
always be type(uint).max
to get a quote which gives amoutADesired
because the intention (in MagicSwapV2Router:addLiquidityNFT()
) is always to get exactly amountAMinted
.Issue will be addressed on the frontend with UI setting type(uint).max as _amountBDesired without a contract enforcing that value.
In UniswapV2Pair
, when there is a price change, the implementation records the last price as part of the accumulated prices to deduce TWAP later.
However, lastPrice
has token1’s decimals which can be intolerably low. For example, if token1 has 6 decimals, prices with more than 6 decimals start to lose precision such as 3 / 7 = 0.428571 (repeated). This can undervalue TWAP causing e.g. a lending application to undervalue the NFT collateral of a borrow.
// function UniswapV2Pair:_update
lastPrice = 10 ** TOKEN0_DECIMALS * _reserve1 / _reserve0;
// write an oracle entry
(observationIndex, observationCardinality) = observations.write(
observationIndex,
_blockTimestamp(),
lastPrice,
observationCardinality,
observationCardinalityNext
);
Remediations to consider
Consider using a scaling factor e.g. 1e18 or 1e27 to ensure there is sufficient precision. Note that the price being scaled should be documented for TWAP users or scale down TWAP in the appropriate methods.
For more info: https://en.wikipedia.org/wiki/Fixed-point_arithmetic#Division
If the result is not exact, the error introduced by the rounding can be reduced or even eliminated by converting the dividend to a smaller scaling factor. For example, if r = 1.23 is represented as 123 with scaling 1/100, and s = 6.25 is represented as 6250 with scaling 1/1000, then simple division of the integers yields 123÷6250 = 0 (rounded) with scaling factor (1/100)/(1/1000) = 10. If r is first converted to 1,230,000 with scaling factor 1/1000000, the result will be 1,230,000÷6250 = 197 (rounded) with scale factor 1/1000 (0.197). The exact value 1.23/6.25 is 0.1968.
The protocol is meant to be used with vaults that always follow 18 decimals convention. If another vault is used with lower decimals, say 6, loss of precision is acceptable. DeFi is using 6 decimal prices with USDC and other tokens safely. Tokens with 0 decimals are not meant to be supported.
In the NftVault
contract, init()
function accepts _collection
array input. Currently, duplicate configuration entries in _collection
input will be processed without raising any error. Later configuration entries will override previous entries with the same collection.addr
value.
Duplicate entries will result in different vaultHash
value in NftVaultFactory:createVault()
and consequently multiple NftVaults being generated. Also, there can be many “duplicate” vaults that have the same collections but in the different order.
Remediations to consider
Check the return value at the following line in NftVault:init()
function and if false revert
allowedCollections.set(collection.addr, nftType);
Perform _collection
array input validation and normalization (checking for duplicates, checking that collection addresses are sorted) in NftVaultFactory:createVault()
before generating vault hash id.
On the product level, it is extremely easy to create a duplicate vault which would be essentially identical for a human (say they differ by one NFT). This logic tries to prevent endless duplicates however it is accepted that if users really want to, they can do it. Vault curation will be on the UI side.
Multiple security advisories overlap with the 4.7.0 version currently used within the project https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories. Update to the most recent patch version 4.7.3.
swapLeftover()
function
In the MagicSwapV2Router
contract, in the swapLeftover()
function, there are two unnecessary casts of _tokenA
variable which is of address type to address.
1. path[0] = address(_tokenA);
2. _approveIfNeeded(address(_tokenA), _amountIn);
override
keyword to the interface functions
Add override
keyword to functions which are implementation of functions declared in parent interfaces to differentiate interface-related functionality from contract-specific functionality more explicitly. For example, add an override
keyword to functions in the UniswapV2Factory
contract which are implementations of corresponding functions declared in the IUniswapV2Factory
interface (also NftVault
and INftVault
). This also allows detection of functions that are not properly declared in the parent interface.
In the NftVaultFactory
and corresponding interface, getVaultAt()
and getPermissionedVaultAt()
are defined with insufficiently descriptive argument name _i
. Consider using _vaultId
, _id
, or _index
.
Following condition in the Oracle:grow()
function is not needed since observationCardinalityNext
which is assigned to the current
variable can never be 0. observationCardinalityNext
is initialized to 1 during contract deployment.
require(current > 0, 'I');
In addition, the following condition in the Oracle:observe()
is unnecessary since observationCardinality
can never be 0. It is initialized to 1 during contract deploy.
require(cardinality > 0, 'I');
Oracle code inherited checks from original code and the goal was to keep it as close to original as possible.
Throughout the codebase, different approaches for raising errors are used. In the NftVault
contract Solidity custom errors are used. In UniswapV2Factory
and UniswapV2Pair
contracts, string errors are raised in require
expressions. While in the MagicSwapV2Router
contract, there are instances of both approaches. Consider choosing one approach and consistently applying it throughout the codebase.
Code inherited errors handling from original code and the goal was to keep the code as close to original as possible and use new way of error handling whenever possible.
Indexed event attributes facilitate off-chain tracking and monitoring. Their absence may negatively affect the performance of these off-chain systems.
INftVaultFactory
, the VaultCreated
event definition does not have indexed attributes.INftVault
, the Deposit
, Withdraw
, AllowedDepositWithdraw
, DisallowedDepositWithdraw
, AllowedContract
, and DisallowedContract
event definitions do not have indexed attributes.SafeMath lib is imported in UniswapV2Library, UniswapV2Router02, UniswapV2Pair, and UniswapV2ERC20. This library provides math functions that handle overflow, and underflow edge cases. However, this functionality is built in Solidity versions 0.8.0+. Therefore, in the current implementation, many operations that rely on SafeMath perform redundant checks. Consider, replacing SafeMath functionality with built-in math operations.
It is understood and accepted. We want to keep the code as close to original as possible.
Add warnings to Natspec documentation for INftVault functions, such as deposit()/depositBatch()
and withdraw()/withdrawBatch()
, that they should not be used directly but through MagicSwapV2Router. Otherwise, user transactions could be frontrunned.
src/UniswapV2/libraries/FixedPoint.sol
src/UniswapV2/libraries/UQ112x112.sol
uint224
is not used in UniswapV2Pair
which is what UQ112x112
is forOwnable2Step.sol
in UniswapV2Pair
Counters.sol
in NftVaultFactoryUse consistent pragma version across all files, or add —use
to forge build to specify the solc version or a solc path.
Not comprehensive documentation for Soulbound vaults
In INftVaultFactory.sol
/// @param isSoulbound if true, Vault is soulbound and its ERC20 token can only be transfered
/// to `allowedContracts` managed by `owner`
However, Soulbound ERC20 tokens can also be transferred to EOA or the vault itself. Consider adding documentation about these 2 possibilities.
Misspellings
src/Vault/INftVault.sol
/// @notice Returns true if wallet is allwed to deposit/withdraw. Only applicable to permissioned vault.
src/Vault/NftVault.sol
/// @notice deposit/withdraw allow list. Maps wallet address to bool, if true, wallet is allwed to deposit/withdraw
/// @notice Vault ERC20 receive allow list. Maps contract address to bool, if true, contract is allwed to receive
/// @notice unique it of the vault geenrated using its configuration
bytes32 public VAULT_HASH;
In INftVaultFactory
VaultCreated
createVault
is incorrect since function doesn’t return already deployed vault if one already exists(newly)
since it always newly
_
isPermissionedVault()
isVault()
In IUniswapV2Factory
Update getFeesAndRecipient()
natspec comment, this part especially if the owner of UniswapV2Factory is malicious
, since if this is the case cap fee will be the smallest problem.
Fees are capped by MAX_FEE, however it is possible for a malicious owner
of this contract to do a combination of transactions to achive fees above MAX_FEE.
Missing natspec for some of the functions
getPair()
allPairs()
allPairsLength()
createPair()
PairCreated
The vault, after the constructor()
runs, starts without collections defined. It relies on init()
to set and configure the collections. The current router createVault()
combines the constructor()
and init()
into 1 call.
However, collection initialization can be combined into constructor and the vault won’t be in a partial state even if the router gets updated in the future.
Consider initializing the collections in the constructor.
Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.
Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.
The scope of this report and review is limited to a review of only the code presented by the Treasure DAO team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.