[PoolFactory.sol] createPoolADD() function is payable but does not contain a function to withdraw funds

[code423n4]

Handle

maplesyrup

Vulnerability details

Impact

This is a medium risk vulnerability as it can affect funds within pools that are created via this contract. With no withdraw functions being implemented, it is possible that funds can be locked in the contract with no way to retrieve earnings or previously sent funds.

Proof of Concept

According to Slither Analyzer documentation (https://github.com/crytic/slither/wiki/Detector-Documentation#contracts-that-lock-ether), functions that are considered payable should have a withdraw function in order to receive funds or earnings from the contract.

In this case the function createPoolADD() is stated as payable but no withdraw function is stated. Since this creates a pool, it would make sense that any earnings made from the pool should be allowed to be withdrawn from it. If earnings are not handled in this portion of the contract or if it is simply to create pools then the payable function should be deleted.

Slither Detector:

Contract locking ether found:

Contract PoolFactory (contracts/poolFactory.sol, #6-151)

has payable functions:

PoolFactory.createPoolADD(uint256,uint256,address) (contracts/poolFactory.sol#45-62)

But does not have a function to withdraw the ether

---

Code in PoolFactory.sol

function createPoolADD(uint256 inputBase, uint256 inputToken, address token) external payable returns(address pool) { require(getPool(token) == address(0)); // Must be a valid token require((inputToken > 0 && inputBase >= (10000*10**18)), "!min"); // User must add at least 10,000 SPARTA liquidity & ratio must be finite Pool newPool; address _token = token; if(token == address(0)){_token = WBNB;} // Handle BNB -> WBNB require(_token != BASE && iBEP20(_token).decimals() == 18); // Token must not be SPARTA & it's decimals must be 18 newPool = new Pool(BASE, _token); // Deploy new pool pool = address(newPool); // Get address of new pool mapToken_Pool[_token] = pool; // Record the new pool address in PoolFactory _handleTransferIn(BASE, inputBase, pool); // Transfer SPARTA liquidity to new pool _handleTransferIn(token, inputToken, pool); // Transfer TOKEN liquidity to new pool arrayPools.push(pool); // Add pool address to the pool array arrayTokens.push(_token); // Add token to the listed array isListedPool[pool] = true; // Record pool as currently listed Pool(pool).addForMember(msg.sender); // Perform the liquidity-add for the user emit CreatePool(token, pool); return pool; }

No withdraw function is listed in this function but the function is stated as "payable".

---

Console output (via Slither, JSON format):

"locked-ether": [

"Contract locking ether found:\n\tContract Pool (contracts/Pool.sol#13-364) has payable functions:\n\t - Pool.swapTo(address,address) (contracts/Pool.sol#211-226)\n\tBut does not have a function to withdraw the ether\n",

"Contract locking ether found:\n\tContract PoolFactory (contracts/poolFactory.sol#6-151) has payable functions:\n\t - PoolFactory.createPoolADD(uint256,uint256,address) (contracts/poolFactory.sol#45-62)\n\tBut does not have a function to withdraw the ether\n"

],

Tools Used

Spartan Contracts Solidity (v 0.8.3) Slither Analyzer (v 0.8.0)

Recommended Mitigation Steps

1. Clone repository for Spartan Smart Contracts
2. Create a python virtual environment with a stable python version
3. Install Slither Analyzer on the python VEM
4. Run Slither against all contracts

[verifyfirst]

By design the poolFactory can only createPools. The pool address have their own logic to add and remove user funds.

[ghoul-sol]

Per sponsor comment, invalid