PERMANENT_INITIAL_LIQUIDITY_HOLDER not 100% safe

[code423n4]

Handle

gpersoon

Vulnerability details

Impact

The initial tokens are minted to the address PERMANENT_INITIAL_LIQUIDITY_HOLDER
The comments suggest they can never be moved from there.
However transferFrom in SyntheticToken.sol allows longShort to move tokens from any address so also from address PERMANENT_INITIAL_LIQUIDITY_HOLDER

This is unlikely to happen because the current source of LongShort.sol doesn't allow for this action.
However LongShort.sol is upgradable to in theory a future version could allow this.

Proof of Concept

// https://github.com/code-423n4/2021-08-floatcapital/blob/main/contracts/contracts/LongShort.sol#L34
/// @notice this is the address that permanently locked initial liquidity for markets is held by.
/// These tokens will never move so market can never have zero liquidity on a side.
/// @dev f10a7 spells float in hex - for fun - important part is that the private key for this address in not known.
address public constant PERMANENT_INITIAL_LIQUIDITY_HOLDER = 0xf10A7_F10A7_f10A7_F10a7_F10A7_f10a7_F10A7_f10a7;

//https://github.com/code-423n4/2021-08-floatcapital/blob/main/contracts/contracts/LongShort.sol#L304
function _seedMarketInitially(uint256 initialMarketSeedForEachMarketSide, uint32 marketIndex) internal
...
ISyntheticToken(syntheticTokens[latestMarket][true]).mint(PERMANENT_INITIAL_LIQUIDITY_HOLDER,initialMarketSeedForEachMarketSide);
ISyntheticToken(syntheticTokens[latestMarket][false]).mint(PERMANENT_INITIAL_LIQUIDITY_HOLDER, initialMarketSeedForEachMarketSide);

// https://github.com/code-423n4/2021-08-floatcapital/blob/main/contracts/contracts/SyntheticToken.sol#L91
function transferFrom(address sender, address recipient, uint256 amount) public override returns (bool) {
if (recipient == longShort && msg.sender == longShort) { // sender could be any address
super._transfer(sender, recipient, amount);
return true;
} else {
return super.transferFrom(sender, recipient, amount);
}
}

Tools Used

Recommended Mitigation Steps

Accept the risk and document this in the contract
or update transferFrom to contain the following:
if (recipient == longShort && msg.sender == longShort && sender!=PERMANENT_INITIAL_LIQUIDITY_HOLDER) {

[JasoonS]

Note, even if we were to remove this initial liquidity there is no issue as long as the market has users on both sides.

This initial liquidity is solely for the first few minutes (or however long) of the market when there is no external users yet.

If the market ever has zero users (zero additional liquidity) and we were to somehow force those last tokens to be redeemed, 'zero users' would care that the market has collapsed and is non-functional anymore.

[moose-code]

Interesting point but its way too far fetched we upgrade the smart contract to include a function that would ever call transferFrom from the very awkward address 0xf10A7_F10A7_f10A7_F10a7_F10A7_f10a7_F10A7_f10a7

[JasoonS]

tldr: there are far worse things that we could upgrade into the contracts than this where there is basically nothing to gain anyway.

[0xean]

Leaving as 1, sponsor acknowledges it's an edge case that could cause non functioning markets which certainly qualifies as "state handling" in

1 — Low: Low: Assets are not at risk. State handling, function incorrect as to spec, issues with comments.