0xsequence / niftyswap Goto Github PK
View Code? Open in Web Editor NEWNiftyswap - decentralized swap protocol for ERC-1155 tokens, inspired by Uniswap
Home Page: https://niftyswap.io
License: Other
Niftyswap - decentralized swap protocol for ERC-1155 tokens, inspired by Uniswap
Home Page: https://niftyswap.io
License: Other
by @Agusx1211
Presently, NiftyswapExchange.sol is implemented to pool/swap 1155:1155 pairs, which is very cool and useful. As discussed, it's convenient to interface with ERC20's such as having a DAI:1155 niftyswap pool or USDC:1155 pool, etc.
I propose to leave NiftyswapExchange.sol as it is (1155:1155), and write a variant as NiftyswapExchange20.sol for 20:1155 pairs, where the base currency is an ERC20 token.
By @Agusx1211
The contracts don't allow users to perform trades using multiple pools with a single transaction; this could be partionally fixed by enabling the users to customize the
_data
field when transferring the newly acquired tokens, it could contain an aditional trade to be performed on another exchange.It has to be noted that such a solution only works to enable trades between different exchange contracts. However, trades of different IDs on the same exchange contract would still not be possible, because of the usage of the reentrancy-lock.
By @Agusx1211
The
NiftyswapExchange
contracts always round transactions in a way that's favorable to the existing liquidity providers; this avoids them paying for inefficiencies when trading or adding liquidity, further aligning all incentives. This rounding it's always performed by increasing the resulting amount by1
, although sufficient, it may lead to rounding calculations that don't require it.Consider replacing it with a more precise
divRound
methoda / b + a % b == 0 ? 0 : 1
By @Agusx1211
The issue occurs when an exchange is created with the same contract for baseToken and token; the system allows anyone to create a pool with base[baseTokenID]/token[ID], and allows ID to be equal to baseTokenID, hence the creation of a pool of base/base.
When the exchange rate of a pool gets calculated, the contract assumes that all its token balance corresponds to the pool, but this is not the case if the pair is base/base, in this case, the balance of the contract includes the provided liquidity for all the other pairs.
This miscalculation of _getTokenReserves opens the possibility of executing two different exploits, one of such exploits it's performing trades against the base/base pair, and the other is the use of removeLiquidity to take the liquidity of all the pools at once.
By @Agusx1211
Currently the contracts (For example https://github.com/0xsequence/niftyswap/blob/master/src/contracts/exchange/NiftyswapExchange.sol) sepecifies the compier to be
pragma solidity 0.7.4;
I was wandering If it can be upgraded to ^0.8.0 (Because Iam using this version in my project).
Thanks :)
If any of the two contracts of the tokens encounters an error and it stops being able to perform transfers the whole pool it's affected, making the associated funds on the other token impossible to withdraw.
Consider adding an aditional
removeLiquidity
parameter that allows the withdrawal of onlybase
or onlytoken
.Similar issuees had happened on Uniswap before (https://twitter.com/UniswapExchange/status/1072286773554876416) and (https://blog.synthetix.io/guide-on-migrating-seth-liquidity/)
By @Agusx1211
Almost all NFT exchanges allow for the creators to receive part of the trading fee, but Niftyswap currently doesn't.
As a V1, we could add two parameters in an exchange contract ; creatorFee and creatorAddress. The former is the % of fee that would go to the creator and the latter would be the address where these fees would be sent to. In order to prevent capture by front runners and such, these two parameters should be set by an operator.
Specifications:
creatorAddress
and creatorFee
. Both default to 0x0
on exchange creationcreatorAddress
and creatorFee
.creatorFee is > 0
, an additional fee is charged in the ERC-20 currency and sent to the creatorAddress
The
onERC1155BatchReceived
method on the exchange uses assembly code to retrieve the first 4 bytes of the_data
; this code can be simplified by usingabi.decode(_data, (bytes4))
although less efficient (29 GAS approx), it's simpler to review, and audit.
By @Agusx1211
it would be great if we can export these constants so that consumers can directly import them from the released package
niftyswap/tests/utils/helpers.ts
Lines 40 to 84 in f0aba0b
by @Agusx1211
By @Agusx1211
We are currently partnering with Gitcoin to provide bounties for vulnerabilities in our contracts. The rewards are small as these contracts have not yet been audited. The bounties size will be reviewed upward once the audits have been performed.
We of course want to know every vulnerability, but in particular:
Decisions on the eligibility and size of a reward are the sole discretion of Horizon Games.
Public disclosure of a vulnerability makes it ineligible for a bounty. Instead issues must be submitted to [email protected].
Issues must be new to the team. They can’t have already been identified by another user or by an audit.
No employees, contractors or others with current or prior commercial relationships with Horizon Games are eligible for rewards.
Provide the steps required to demonstrate an issue. If we cannot reproduce an issue we will not be able to reward it.
The size of the bounty will vary depending on the severity of the issue discovered. The severity is calculated according to the OWASP risk rating model based on Impact and Likelihood.
Decisions on the eligibility and size of a reward are guided by the rules above, but are, in the end, determined at the sole discretion of Horizon Games
Critical: up to $3,000
High: up to $1,500
Medium: up to $500
Low: up to $250
In addition to severity, other variables are also considered when Horizon evaluates the eligibility and size of a bounty, including (but not limited to):
Quality of description
Higher rewards are paid for clear, well-written submissions.
Quality of reproducibility
Please include test code, scripts and detailed instructions. The easier it is for us to reproduce and verify the vulnerability, the higher the reward.
Quality of fix, if included
Higher rewards are paid for submissions with clear description of how to fix the issue.
By @Agusx1211
Hi this is on the sol-update
branch
Heres some code:
NiftyswapFactory20 internal immutable swapFactory;
...
//
// in constructor
//
swapFactory = new NiftyswapFactory20(address(this));
swapFactory.createExchange(address(this), WETH9, 10, 0);
uint256[] memory amount = new uint256[](1);
amount[0] = 10000000000;
bytes memory data = abi.encode(
ADDLIQUIDITY_SIG,
AddLiquidityObj(amount, block.timestamp + SECONDS_DEADLINE)
);
safeBatchTransferFrom(
address(this),
address(swapFactory),
tokenID,
amount,
data
);
...
// in a function for buying the token
swapFactory.getPairExchanges(address(this), WETH9)[0].buyTokens(...);
This last line errors with this:
TypeError: Member "buyTokens" not found or not visible after argument-dependent lookup in address.
I'd just like to know how to fix it.
As per the comment at https://github.com/arcadeum/niftyswap/blob/c4c94e718e4195b4b29b2e69f9b65132dfa6456c/contracts/exchange/NiftyswapFactory.sol#L20 the current design permits for only one niftyswap exchange per ERC-1155 token, which could lead to some ERC-1155 tokens being "blocked".
As per @Agusx1211 :
The contract factory uses the mapping
tokenToExchange
to avoid the creation of duplicated pools for the sametoken
; however, the_baseTokenAddr
token of each exchange is not defined on the factory and instead is provided as one of the input parameters of thecreateExchange
function.Because the calling of
createExchange
is not permissioned, an attacker could create exchanges with random or broken base addresses, blocking the genuine creation of the exchanges.Consider adding an aditional dimension to the
tokenToExchange
mapping and only enforce uniquebase/token
combinations.
The exchange defines a function signature for self-deposit that's used during an
addLiquidty
call, to accept the base tokens.The exchange doesn't check that the operator of the self-deposit transfer it's, in fact, itself, neither that the self-deposit token is the base token.
This allows depositing any ERC1155 token on the exchange, as long as the data contains the self-deposit signature.
By @Agusx1211
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.