# Sudoswap SharedPools Review Log

The SharedPools contract is a contract designed to allow and easy user interface to the SudoSwap v2 nft dex contract, it attempts to model the results of a simple uniswapp v2 pool.

The review was preformed over 2 days and uses the commit hash 7f45e3f in https://github.com/sudoswap/shared-pool. The review covers only the shared pools mechanics not any details of the v2 dex.

## Issues

Summary: Critical: 1, High: 1, Medium 1, Low: 1, Note: Various

Presented in chronological order.

* Redeem's fractional sales calculate against the wrong pool size: When a position is redeemed the user may get fractional NFT tokens but these cannot be turned into actual NFTs as NFTs are non fungible, so the pool attempts to exit with a rounded number of NFTs. The fractional part is either sold or 1 - fractional part is bought from the curve to get the rounded number of NFTs. In the case where it is rounded up the fraction is sold against a pool with liquidity equal to what the pool would have if the non fractional part is removed. This is correct behavior, however in two other cases this adjustment isn't made and the user exiting sells against the pool's liquidity before their exit. This breaks the AMM invariant as it means that while the tangent price is correct for an infinitesimally small trade, that the sales do not have properly accounted for slippage or size impacts. For example if a user withdraws 100 USDC and 10.5 NFTs from a pool of size (101, 10.605) the trade executing a sale of 0.5 nfts against a pool of (101, 10.605) has very high slippage whereas a sale of 0.5 NFTs against a pool of size (1, 0.105) would have very high slippage. This effect is small in average cases but does negatively impact the remaining LPs who subsidze this bad trade, and in an narrow subset of cases can be used to do a curvature rebound attack.
* Severity Estimate: High [Likelihood: High, Impact: Medium]
* Remediation Recommendation: The pool should be adjusted by the non fractional part of the withdraw before the fractional part is withdrawn in all cases.
* Gas Cost Refunds Use an incorrect constant: The `_pullTokensFromSender` in the eth mixin method uses a constant to estimate the cost of sending a refund to the user, that constant is 21,000 * block.basefee. This is both an under and over estimate of the cost to refund the user, it is an over estimate because a call with nonzero value is 9k gas not 21k gas, and an under estimate because the user specifies a priority fee in addition to the base cost. So if a user sets a very high priority fee relative to the base fee they can be refunded an amount which costs them more money than is sent. Conversely if the priority fee is zero, then a user can be not refunded for an overpayment of 11k gas, that lack of refund can cause a user to loose several dollars in a median case of gas fees on eth mainnet.
* Severity Estimate: Medium [Likelihood: High, Impact: Low]
* Remediation Recommendation: This refund logic should be optimized to more closely match the real costs
* Improperly configured royalty addresses prevent some withdraws: The redeem function requires transactions be made which pay out the royalty amount which is expected for the sale of the fractional part of the withdraw. The ETH mixin uses the solmate safe eth transfer function which will revert if the call fails. Given this if a royalty address is a smart contract is misconfigured to not have receive function or to revert for any other reason in it's receive function then all withdraws which have a fractional part will brick. Users may still be able to exit by trading to a perfect ratio then withdrawing.
* Severity Estimate: Low [Likelihood: Low, Impact: Medium]
* Remediation Recommendation: Don't require transfer success
* Assembly loading of token's immutable location is incorrect: The contracts use a version of a proxy which will pass forward immutable variables at specific locations in calldata. When the ERC20 pool tries to load the value of the token it uses `sub(shr(240, calldataload(sub(calldatasize(), 2))), 20)` which will underflow and produce a very large number. The code then loads calldata from this number using assembly, producing 0. This makes the code non functional as it will try to call transfer and transferFrom on 0.
* Severity Estimate: Critical [Likelihood: High, Impact: High]
* Remediation Recommendation: Use the same strategy as the other immutable data loads, and load from a constant location.

#### Notes

* Slightly odd/inefficient for the factory to enforce byte string lengths less than 32 bytes and then to force cast the bytes strings into bytes32 types. Would make more sense to just provide bytes32 types as inputs to the factory.