/
QA Report
Abhan profile imageAbhan
QA

1. Wrong check of listing type in listVesting

  • In listVesting, there is a require check which should make sure that if listing type is partial then _minPurchaseAmt < 0 and _minPurchaseAmt <= amount.

https://github.com/code-423n4/2024-12-secondswap/blob/214849c3517eb26b31fe194bceae65cb0f52d2c0/contracts/SecondSwap_Marketplace.sol#L252C9-L255C11

function listVesting( address _vestingPlan, uint256 _amount, uint256 _price, uint256 _discountPct, ListingType _listingType, DiscountType _discountType, uint256 _maxWhitelist, address _currency, uint256 _minPurchaseAmt, bool _isPrivate ) external isFreeze { require( _listingType != ListingType.SINGLE || (_minPurchaseAmt > 0 && _minPurchaseAmt <= _amount), "SS_Marketplace: Minimum Purchase Amount cannot be more than listing amount" ); ... }
  • But here if listing type is partial then it will be true in first check which bypass this require statment and _minPurchaseAmt will not be checked if it is partial listing. That means minimum purchase amount can be more than listing amount.

  • Protocol should check by another require statement that if listing type is partial then _minPurchaseAmt < 0 and _minPurchaseAmt <= amount.

2. No option to remove an exsting support token from isTokenSupport

  • Admin can add a token for use it as a currency to buy vesting tokens by calling addCoin.
function addCoin(address _token) external { require( msg.sender == IMarketplaceSetting(marketplaceSetting).s2Admin(), "SS_Marketplace: Unauthorized user" ); require( !isTokenSupport[_token], "SS_Marketplace: Token is currently supported" ); isTokenSupport[_token] = true; }
  • If admin wants to remove particular token because of something malicious activity or something else, admin have no option to remove token from this mapping and it can be used forever.
-function addCoin(address _token) external { +function addOrRemoveCoin(address _token, bool _supported) external { require( msg.sender == IMarketplaceSetting(marketplaceSetting).s2Admin(), "SS_Marketplace: Unauthorized user" ); require( !isTokenSupport[_token], "SS_Marketplace: Token is currently supported" ); - isTokenSupport[_token] = true; + isTokenSupport[_token] = _supported; }

3. maxSellPercent can be more than 100%.

  • setMaxSellPercent has no check which make sure that maxSellPercent cannot be more than 100%.
function setMaxSellPercent( address vesting, uint256 maxSellPercent ) external { require( SecondSwap_StepVesting(vesting).tokenIssuer() == msg.sender, "SS_VestingManager: Invalid Token Issuer" ); vestingSettings[vesting].maxSellPercent = maxSellPercent; emit MaxSellPercentUpdated(vesting, maxSellPercent); }
  • In every other function of protocol there are sufficient checks but this function is missing important check which can lead to unintended behaviours.
function setMaxSellPercent( address vesting, uint256 maxSellPercent ) external { require( SecondSwap_StepVesting(vesting).tokenIssuer() == msg.sender, "SS_VestingManager: Invalid Token Issuer" ); + require(maxSellPercent <= 10000, "cannot exceed 100%"); vestingSettings[vesting].maxSellPercent = maxSellPercent; emit MaxSellPercentUpdated(vesting, maxSellPercent); }

4. vestingId can be same for multiple vestings

  • When token issuer deploy vesting there is a param of vestingId as a string and which is emitted in the event of VestingDeployed.

https://github.com/code-423n4/2024-12-secondswap/blob/214849c3517eb26b31fe194bceae65cb0f52d2c0/contracts/SecondSwap_VestingDeployer.sol#L104C5-L132C6

function deployVesting( address tokenAddress, uint256 startTime, uint256 endTime, uint256 steps, string memory vestingId ) { ... emit VestingDeployed(tokenAddress, newVesting, vestingId); }
  • This vestingId can be same for multiple vestings beacuse there is no check which make sure that this vestingId is already used for some other vesting. This can mislead to the user on front-end side.

  • Protocol should make sure that same vestingId should not used for multiple vestings.

5. Listing amount does not matter in case of penalty

  • If userA list 1000 tokens and unlist before minimum duration userA will have to pay penalty of x amount.

https://github.com/code-423n4/2024-12-secondswap/blob/214849c3517eb26b31fe194bceae65cb0f52d2c0/contracts/SecondSwap_Marketplace.sol#L357C1-L359C19

  • If userB list 100 tokens and unlist before minimum duration userB will have to pay same penalty and if userB list 1000 tokens with 10 listing with each have 100 tokens and unlist before minimum duration, userB have to pay 10x penalty which is unfair.
  • Penalty should be in percentage and it should be charged on listing amount.
  • Therefore high amount of listing will have to pay high penalty and vice versa for low amount listing.

6. No Access control in whitelisting

  • When user wants to create a private listing, there is a whitelisting mechnism.
  • However there is no access control in whitelisting which means anyone can whitelist ownself.
function whitelistAddress() external { require( totalWhitelist < maxWhitelist, "SS_Whitelist: Reached whitelist limit" ); require( userSettings[msg.sender] == false, "SS_Whitelist: User is whitelisted" ); userSettings[msg.sender] = true; totalWhitelist++; emit WhitelistedAddress(totalWhitelist, msg.sender); }
  • That means there is no sense of whitelisting and it is not private anymore.
  • Protocol must implement proper whitelisting mechnism.

7. MarketPlace and vesting contracts are not upgradable

  • As per the readme Attack ideas (where to focus for bugs) marketplace and vesting contract should be upgradable.
  • But in contracts of marketplace and vesting, there is no proxy implemnented.
  • That means contracts are not upgradable.