Summary

A vulnerability exists in the SecondSwap_VestingManager contract where the setSellable function can inadvertently reset vesting settings to default values. The function uses maxSellPercent == 0 as the sole condition to determine if default values should be initialized, without distinguishing between new vesting contracts and existing ones whose settings may have been legitimately modified.

This creates a security issue where a token issuer can force a reset of vesting settings by first setting maxSellPercent to 0 through setMaxSellPercent, then having an admin call setSellable. This breaks the intended privilege separation and can disrupt active trading by unexpectedly changing fee structures and selling limits.

The vulnerability has significant protocol impact as fees set to -1 have special meaning (use default marketplace fees), making unintended resets particularly disruptive to the marketplace's economic model.

Proof of Concept

The vulnerable code path:

// Token issuer can trigger this
function setMaxSellPercent(address vesting, uint256 maxSellPercent) external {
    require(SecondSwap_StepVesting(vesting).tokenIssuer() == msg.sender, "SS_VestingManager: Invalid Token Issuer");
    vestingSettings[vesting].maxSellPercent = maxSellPercent;  // Can set to 0
}

// Admin/deployer can then trigger reset
function setSellable(address vesting, bool sellable) external {
    require(s2Admin == msg.sender || vestingDeployer == msg.sender, "SS_VestingManager: Unauthorised user");
    VestingSettings storage vestingSetting = vestingSettings[vesting];
    vestingSetting.sellable = sellable;
    
    // Triggered when maxSellPercent is 0
    if (vestingSetting.maxSellPercent == 0 && vestingSetting.sellable) {
        vestingSetting.maxSellPercent = 2000;
        vestingSetting.buyerFee = -1;
        vestingSetting.sellerFee = -1;
    }
}

Scenario:

// Initial state
vestingSettings[vesting] = VestingSettings({
    sellable: true,
    maxSellPercent: 5000,  // 50%
    buyerFee: 100,         // 1%
    sellerFee: 200         // 2%
});


1. tokenIssuer.setMaxSellPercent(vesting, 0);
2. admin.setSellable(vesting, true);

// Final state
vestingSettings[vesting] = VestingSettings({
    sellable: true,
    maxSellPercent: 2000,  // Reset to 20%
    buyerFee: -1,          // Reset to default
    sellerFee: -1          // Reset to default
});

Recommended Mitigation

1. Add initialization tracking:

mapping(address => bool) private initializedVestings;

function setSellable(address vesting, bool sellable) external {
    require(s2Admin == msg.sender || vestingDeployer == msg.sender, "SS_VestingManager: Unauthorised user");
    
    VestingSettings storage vestingSetting = vestingSettings[vesting];
    vestingSetting.sellable = sellable;
    
    // Only initialize if never initialized before
    if (!initializedVestings[vesting] && vestingSetting.sellable) {
        vestingSetting.maxSellPercent = 2000;
        vestingSetting.buyerFee = -1;
        vestingSetting.sellerFee = -1;
        initializedVestings[vesting] = true;
        emit MaxSellPercentUpdated(vesting, 2000);
    }
    
    emit VestingSellableUpdated(vesting, sellable);
}

2. Add separate initialization function:

function initializeVestingSettings(address vesting) external {
    require(s2Admin == msg.sender, "SS_VestingManager: Unauthorized");
    require(!initializedVestings[vesting], "SS_VestingManager: Already initialized");
    
    vestingSettings[vesting] = VestingSettings({
        sellable: true,
        maxSellPercent: 2000,
        buyerFee: -1,
        sellerFee: -1
    });
    
    initializedVestings[vesting] = true;
}

These changes ensure settings cannot be accidentally reset after initialization while maintaining the ability to explicitly modify individual settings through their respective setter functions.