search

code-423n4 / 2021-12-nftx-findings

0 stars 0 forks source link

Pool Manager can frontrun fees to 100% and use it to steal the value from users #213

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

pedroais

Vulnerability details

Impact

Pool Manager can front-run entry fee to 100% and users could lose all their deposits

Proof of Concept

Considering : The pool manager is the creator of the pool Anyone can create a pool Manager is not a trusted actor

Anyone can create a pool and get people to join. If there is a big deposit admin could front-run the transaction and set the fee to max which is uint(1 ether) = 10**18 (100% as this is a per token fee).

Function that set fees : https://github.com/code-423n4/2021-12-nftx/blob/194073f750b7e2c9a886ece34b6382b4f1355f36/nftx-protocol-v2/contracts/solidity/NFTXVaultUpgradeable.sol#L119 Max fees are 1 ether : https://github.com/code-423n4/2021-12-nftx/blob/194073f750b7e2c9a886ece34b6382b4f1355f36/nftx-protocol-v2/contracts/solidity/NFTXVaultFactoryUpgradeable.sol#L122

The manager could benefit from this by having other pool assets deposited in staking so he would receive fees in Vtokens and could use them to withdraw the nfts.

Recommended Mitigation Steps

Add a timelock to change fees. In that way, frontrunning wouldn't be possible and users would know the fees they are agreeing with.

0xKiwi commented 2 years ago

Most users aren't on vaults that aren't finalized. We warn users for any vaults that arent finalized and we don't present them on our website. Acknowledging and disagreeing with severity.

dmvt commented 2 years ago

In my view, this is a medium risk. While user funds are at direct risk, the likelihood of this happening or being worth the effort is low. As the sponsor states, it's very rare for a user to interact with an un-finalized vault. The user would have to be directly linked to the vault and then ignore the giant warning presented front and center in the UI. If that warning were to be removed, however, the risk would increase. This external requirement is the only reason I'm going with medium and not low.