immutable `withdraw` + `split` #428

YouStillAlive · 2025-03-06T09:59:51Z

closes #393

github-actions · 2025-03-06T10:02:13Z

Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary

incorrect-equality (1 results) (Medium)
uninitialized-local (1 results) (Medium)
unused-return (1 results) (Medium)
calls-loop (2 results) (Low)
timestamp (4 results) (Low)
dead-code (3 results) (Informational)
naming-convention (13 results) (Informational)
immutable-states (3 results) (Optimization)

incorrect-equality

Impact: Medium
Confidence: High

ID-0
TimedDealProvider._withdraw(uint256,uint256) uses a dangerous strict equality:
- amount == 0

LockDealNFT/contracts/SimpleProviders/TimedDealProvider/TimedDealProvider.sol

Lines 25 to 52 in 6b9f53f

    
           function _withdraw( 
        
               uint256 poolId, 
        
               uint256 amount 
        
           ) internal override firewallProtectedSig(0x9e2bf22c) returns (uint256 withdrawnAmount, bool isFinal) { 
        
               if (amount == 0) return (0, false); 
        
               isFinal = true; 
        
               withdrawnAmount = amount; 
        
               uint256[] memory params = provider.getParams(poolId); 
        
               uint256 remainingAmount = params[0] - amount; 
        
               if (remainingAmount > 0 && lastPoolOwner[poolId] != address(0)) { 
        
                   // create immutable NFT 
        
                   uint256 newPoolId = lockDealNFT.mintForProvider( 
        
                       lastPoolOwner[poolId], 
        
                       lockDealNFT.poolIdToProvider(poolId) 
        
                   ); 
        
                   // clone vault id 
        
                   lockDealNFT.cloneVaultId(newPoolId, poolId); 
        
                   // register new pool 
        
                   params[0] = remainingAmount; 
        
                   provider.registerPool(newPoolId, params); 
        
                   poolIdToTime[newPoolId] = poolIdToTime[poolId]; 
        
                   poolIdToAmount[newPoolId] = poolIdToAmount[poolId]; 
        
                   delete lastPoolOwner[poolId]; 
        
               } 
        
               // Reset and update the original pool 
        
               params[0] = 0; 
        
               provider.registerPool(poolId, params); 
        
           }

uninitialized-local

Impact: Medium
Confidence: Medium

ID-1
LockDealNFTInternal._split(uint256,address,uint256,address).locals is a local variable never initialized

LockDealNFT/contracts/LockDealNFT/LockDealNFTInternal.sol

Line 121 in 6b9f53f

SplitLocals memory locals;

unused-return

Impact: Medium
Confidence: Medium

ID-2
LockDealNFT.mintAndTransfer(address,address,uint256,IProvider) ignores return value by IERC20(token).approve(address(vaultManager),amount)

LockDealNFT/contracts/LockDealNFT/LockDealNFT.sol

Lines 25 to 42 in 6b9f53f

    
           function mintAndTransfer( 
        
               address owner, 
        
               address token, 
        
               uint256 amount, 
        
               IProvider provider 
        
           ) 
        
               external 
        
               firewallProtected 
        
               onlyApprovedContract(address(provider)) 
        
               notZeroAddress(owner) 
        
               notZeroAddress(token) 
        
               notZeroAmount(amount) 
        
               returns (uint256 poolId) 
        
           { 
        
               poolId = _mint(owner, provider); 
        
               IERC20(token).approve(address(vaultManager), amount); 
        
               poolIdToVaultId[poolId] = vaultManager.depositByToken(token, amount); 
        
           }

calls-loop

Impact: Low
Confidence: Medium

ID-3
LockDealNFTState._getData(uint256) has external calls inside a loop: poolInfo = BasePoolInfo(provider,provider.name(),poolId,poolIdToVaultId[poolId],ownerOf(poolId),tokenOf(poolId),provider.getParams(poolId))

LockDealNFT/contracts/LockDealNFT/LockDealNFTState.sol

Lines 43 to 54 in 6b9f53f

    
           function _getData(uint256 poolId) internal view returns (BasePoolInfo memory poolInfo) { 
        
               IProvider provider = poolIdToProvider[poolId]; 
        
               poolInfo = BasePoolInfo( 
        
                   provider, 
        
                   provider.name(), 
        
                   poolId, 
        
                   poolIdToVaultId[poolId], 
        
                   ownerOf(poolId), 
        
                   tokenOf(poolId), 
        
                   provider.getParams(poolId) 
        
               ); 
        
           }

ID-4
LockDealNFTState.tokenOf(uint256) has external calls inside a loop: token = vaultManager.vaultIdToTokenAddress(poolIdToVaultId[poolId])

LockDealNFT/contracts/LockDealNFT/LockDealNFTState.sol

Lines 80 to 82 in 6b9f53f

    
           function tokenOf(uint256 poolId) public view returns (address token) { 
        
               token = vaultManager.vaultIdToTokenAddress(poolIdToVaultId[poolId]); 
        
           }

timestamp

Impact: Low
Confidence: Medium

ID-5
TimedDealProvider._withdraw(uint256,uint256) uses timestamp for comparisons
Dangerous comparisons:
- amount == 0
- remainingAmount > 0 && lastPoolOwner[poolId] != address(0)

LockDealNFT/contracts/SimpleProviders/TimedDealProvider/TimedDealProvider.sol

Lines 25 to 52 in 6b9f53f

    
           function _withdraw( 
        
               uint256 poolId, 
        
               uint256 amount 
        
           ) internal override firewallProtectedSig(0x9e2bf22c) returns (uint256 withdrawnAmount, bool isFinal) { 
        
               if (amount == 0) return (0, false); 
        
               isFinal = true; 
        
               withdrawnAmount = amount; 
        
               uint256[] memory params = provider.getParams(poolId); 
        
               uint256 remainingAmount = params[0] - amount; 
        
               if (remainingAmount > 0 && lastPoolOwner[poolId] != address(0)) { 
        
                   // create immutable NFT 
        
                   uint256 newPoolId = lockDealNFT.mintForProvider( 
        
                       lastPoolOwner[poolId], 
        
                       lockDealNFT.poolIdToProvider(poolId) 
        
                   ); 
        
                   // clone vault id 
        
                   lockDealNFT.cloneVaultId(newPoolId, poolId); 
        
                   // register new pool 
        
                   params[0] = remainingAmount; 
        
                   provider.registerPool(newPoolId, params); 
        
                   poolIdToTime[newPoolId] = poolIdToTime[poolId]; 
        
                   poolIdToAmount[newPoolId] = poolIdToAmount[poolId]; 
        
                   delete lastPoolOwner[poolId]; 
        
               } 
        
               // Reset and update the original pool 
        
               params[0] = 0; 
        
               provider.registerPool(poolId, params); 
        
           }

ID-6
TimedDealProvider.getWithdrawableAmount(uint256) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp < startTime
- finishTime <= block.timestamp

LockDealNFT/contracts/SimpleProviders/TimedDealProvider/TimedDealProvider.sol

Lines 54 to 68 in 6b9f53f

    
           function getWithdrawableAmount(uint256 poolId) public view override returns (uint256) { 
        
               uint256[] memory params = getParams(poolId); 
        
               uint256 leftAmount = params[0]; 
        
               uint256 startTime = params[1]; 
        
               uint256 finishTime = params[2]; 
        
               uint256 startAmount = params[3]; 
        
               if (block.timestamp < startTime) return 0; 
        
               if (finishTime <= block.timestamp) return leftAmount; 
        
               uint256 totalPoolDuration = finishTime - startTime; 
        
               uint256 timePassed = block.timestamp - startTime; 
        
               uint256 debitableAmount = (startAmount * timePassed) / totalPoolDuration; 
        
               return debitableAmount - (startAmount - leftAmount); 
        
           }

ID-7
LockDealNFTInternal._update(address,uint256,address) uses timestamp for comparisons
Dangerous comparisons:
- require(bool,string)(vaultManager.vaultIdToTradeStartTime(poolIdToVaultId[poolId]) < block.timestamp,Can't transfer before trade start time)

LockDealNFT/contracts/LockDealNFT/LockDealNFTInternal.sol

Lines 11 to 28 in 6b9f53f

    
           function _update( 
        
               address to, 
        
               uint256 poolId, 
        
               address auth 
        
           ) internal override firewallProtectedSig(0x30e0789e) returns (address from) { 
        
               if (auth != address(0) && ERC165Checker.supportsInterface(address(poolIdToProvider[poolId]), type(IBeforeTransfer).interfaceId)) { 
        
                   IBeforeTransfer(address(poolIdToProvider[poolId])).beforeTransfer(auth, to, poolId); 
        
               } 
        
               // check for split and withdraw transfers 
        
               if (auth != address(0) && !(approvedContracts[to] || approvedContracts[auth])) { 
        
                   require(approvedPoolUserTransfers[auth], "Pool transfer not approved by user"); 
        
                   require( 
        
                       vaultManager.vaultIdToTradeStartTime(poolIdToVaultId[poolId]) < block.timestamp, 
        
                       "Can't transfer before trade start time" 
        
                   ); 
        
               } 
        
               from = super._update(to, poolId, auth); 
        
           }

ID-8
LockDealProvider.getWithdrawableAmount(uint256) uses timestamp for comparisons
Dangerous comparisons:
- poolIdToTime[poolId] <= block.timestamp

LockDealNFT/contracts/SimpleProviders/LockProvider/LockDealProvider.sol

Lines 56 to 58 in 6b9f53f

    
           function getWithdrawableAmount(uint256 poolId) public view override returns (uint256) { 
        
               return poolIdToTime[poolId] <= block.timestamp ? provider.getWithdrawableAmount(poolId) : 0; 
        
           }

dead-code

Impact: Informational
Confidence: Medium

ID-9
ProviderModifiers._validProvider(uint256,IProvider) is never used and should be removed

LockDealNFT/contracts/SimpleProviders/Provider/ProviderModifiers.sol

Lines 42 to 44 in 6b9f53f

    
           function _validProvider(uint256 poolId, IProvider provider) internal view { 
        
               require(lockDealNFT.poolIdToProvider(poolId) == provider, "Invalid provider poolId"); 
        
           }

ID-10
ProviderModifiers._validProviderInterface(IProvider,bytes4) is never used and should be removed

LockDealNFT/contracts/SimpleProviders/Provider/ProviderModifiers.sol

Lines 58 to 60 in 6b9f53f

    
           function _validProviderInterface(IProvider provider, bytes4 interfaceId) internal view { 
        
               require(ERC165Checker.supportsInterface(address(provider), interfaceId), "invalid provider type"); 
        
           }

ID-11
BasicProvider._withdraw(uint256,uint256) is never used and should be removed

LockDealNFT/contracts/SimpleProviders/Provider/BasicProvider.sol

Lines 65 to 68 in 6b9f53f

    
           function _withdraw( 
        
               uint256 poolId, 
        
               uint256 amount 
        
           ) internal virtual returns (uint256 withdrawnAmount, bool isFinal) {}

naming-convention

Impact: Informational
Confidence: High

ID-12
Function ERC20Token.FreeTest() is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/token/ERC20Token.sol#L22-L24

ID-13
Parameter Array.KeepNElementsInArray(uint256[],uint256)._n is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L11

ID-14
Parameter Array.KeepNElementsInArray(address[],uint256)._arr is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L25

ID-15
Parameter Array.addIfNotExsist(address[],address)._elem is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L81

ID-16
Parameter Array.addIfNotExsist(address[],address)._arr is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L81

ID-17
Function Array.KeepNElementsInArray(address[],uint256) is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L25-L37

ID-18
Function Array.KeepNElementsInArray(uint256[],uint256) is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L11-L23

ID-19
Parameter Array.KeepNElementsInArray(address[],uint256)._n is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L25

ID-20
Parameter Array.isInArray(address[],address)._elem is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L70

ID-21
Parameter Array.KeepNElementsInArray(uint256[],uint256)._arr is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L11

ID-22
Parameter Array.getArraySum(uint256[])._array is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L57

ID-23
Parameter Array.isArrayOrdered(uint256[])._arr is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L40

ID-24
Parameter Array.isInArray(address[],address)._arr is not in mixedCase

https://github.com/The-Poolz/LockDealNFT/blob/6b9f53fbfda372a2e19f436c766404a0d23ec0ab/node_modules/@poolzfinance/poolz-helper-v2/contracts/Array.sol#L70

immutable-states

Impact: Optimization
Confidence: High

ID-25
ProviderState.lockDealNFT should be immutable

LockDealNFT/contracts/SimpleProviders/Provider/ProviderState.sol

Line 9 in 6b9f53f

ILockDealNFT public lockDealNFT;

ID-26
LockDealState.provider should be immutable

LockDealNFT/contracts/SimpleProviders/LockProvider/LockDealState.sol

Line 9 in 6b9f53f

ISimpleProvider public provider;

ID-27
LockDealNFTState.vaultManager should be immutable

LockDealNFT/contracts/LockDealNFT/LockDealNFTState.sol

Line 18 in 6b9f53f

IVaultManager public vaultManager;

github-actions · 2025-03-07T10:52:33Z

Methods

Symbol	Meaning
◯	Execution gas for this method does not include intrinsic gas overhead
△	Cost was non-zero but below the precision setting for the currency display (see options)

	Min	Max	Avg	Calls	usd avg
DealProvider
createNewPool(address[],uint256[],bytes)	258,262	289,062	269,821	66	0.78
ERC20Token
approve(address,uint256)	-	-	45,928	1	0.13
LockDealNFT
approvePoolTransfers(bool)	22,706	44,618	33,662	8	0.10
renounceOwnership()	-	-	23,722	1	0.07
safeTransferFrom(address,address,uint256,bytes)	482,309	721,390	607,831	16	1.76
safeTransferFrom(address,address,uint256)	114,466	419,277	257,144	14	0.74
setApprovedContract(address,bool)	28,264	48,164	44,432	16	0.13
setBaseURI(string)	50,883	79,069	69,674	3	0.20
transferFrom(address,address,uint256)	96,627	114,960	105,794	2	0.31
transferOwnership(address)	-	-	29,439	1	0.09
updateAllMetadata()	-	-	24,683	2	0.07
LockDealProvider
createNewPool(address[],uint256[],bytes)	293,559	304,759	304,170	19	0.88
MockProvider
createNewPool(address[],uint256[],bytes)	-	-	377,637	5	1.09
createNewPoolWithTransfer(address[],uint256[])	-	-	340,743	1	0.99
withdraw(uint256,uint256)	74,299	76,477	75,388	2	0.22
MockTransfer
createNewPool(address[],uint256[],bytes)	-	-	269,462	1	0.78
MockVaultManager
setTransferStatus(bool)	21,710	43,622	32,666	2	0.09
TimedDealProvider
createNewPool(address[],uint256[],bytes)	354,367	383,102	366,281	37	1.06

Deployments

	Min	Max	Avg	Block %	usd avg
DealProvider	-	-	1,730,358	1.3 %	5.00
ERC20Token	-	-	658,352	0.5 %	1.90
LockDealNFT	-	-	4,361,099	3.4 %	12.61
LockDealProvider	-	-	1,667,308	1.3 %	4.82
MockProvider	-	-	860,176	0.7 %	2.49
MockTransfer	-	-	1,759,560	1.4 %	5.09
MockVaultManager	-	-	349,644	0.3 %	1.01
TimedDealProvider	2,018,768	2,018,780	2,018,774	1.6 %	5.84

Solidity and Network Config

Settings	Value
Solidity: version	0.8.25
Solidity: optimized	true
Solidity: runs	200
Solidity: viaIR	true
Block Limit	130,000,000
L1 Gas Price	5 gwei
Token Price	578.41 usd/bnb
Network	BINANCE
Toolchain	hardhat

codecov · 2025-03-07T10:52:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.97%. Comparing base (529fb15) to head (dd0bc29).
Report is 21 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #428      +/-   ##
==========================================
+ Coverage   85.75%   88.97%   +3.21%     
==========================================
  Files          13       13              
  Lines         379      408      +29     
  Branches       91       67      -24     
==========================================
+ Hits          325      363      +38     
+ Misses         53       44       -9     
  Partials        1        1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Lomet · 2025-03-10T11:31:15Z

contracts/SimpleProviders/TimedDealProvider/TimedDealProvider.sol

@@ -20,12 +21,23 @@ contract TimedDealProvider is LockDealState, DealProviderState, BasicProvider {
        lockDealNFT = _lockDealNFT;
        name = "TimedDealProvider";
    }
-
+
    function _withdraw(
        uint256 poolId,
        uint256 amount
    ) internal override firewallProtectedSig(0x9e2bf22c) returns (uint256 withdrawnAmount, bool isFinal) {
        (withdrawnAmount, isFinal) = provider.withdraw(poolId, amount);


we need to skip this line.
on current version we make :
old.amount -= amount
new.amount = old.amount
old.amount = 0(missing)

lets set the new directly to old.amount - amount
and the old to 0

the if on line 31 can be simple

YouStillAlive · 2025-03-11T11:35:02Z

Gas tracking

Results from Hardhat Gas Reporter

Original TimedDealProvider

TimedDealProvider Withdraw	Avg	Calls
now >= startTime && now < finishTime	164,814	800
now > finishTime	167,469	800

Immutable version (version that was deployed to the testnet)

TimedDealProvider Withdraw	Avg	Calls
now >= startTime && now < finishTime	456,600	800
now > finishTime	192,073	800

Immutable version + "viaIR: true" setting

TimedDealProvider Withdraw	Avg	Calls
now >= startTime && now < finishTime	451,988	800
now > finishTime	187,014	800

Immutable version 2

TimedDealProvider Withdraw	Avg	Calls
now >= startTime && now < finishTime	450,874	800
now > finishTime	194,954	800

PR link

Immutable version 3

TimedDealProvider Withdraw	Avg	Calls
now >= startTime && now < finishTime	448,663	800
now > finishTime	193,502	800

PR link

Immutable version 4

TimedDealProvider Withdraw	Avg	Calls
now >= startTime && now < finishTime	430,742	800
now > finishTime	187,098	800

PR link

Immutable version 5

TimedDealProvider Withdraw	Avg	Calls
now >= startTime && now < finishTime	429,291	800
now > finishTime	193,737	800

PR link

Issue 393 `version 5`

immutable withdraw

21f1be0

YouStillAlive marked this pull request as draft March 6, 2025 09:59

YouStillAlive added 4 commits March 6, 2025 12:50

add IBeforeTransfer support

c053892

update withdraw logic

f1cd4e9

update split logic

8fafd83

add comments

c211b5e

YouStillAlive changed the title ~~immutable withdraw~~ immutable withdraw + split Mar 6, 2025

YouStillAlive added 3 commits March 7, 2025 11:41

fix his bug and withdraw data bug

a43f3e4

refactor _mintNewNFT

52de6a6

save higher level of cascade

f35a15b

fix bug after full withdrawal: prevent empty NFT creation

0499523

YouStillAlive marked this pull request as ready for review March 7, 2025 11:40

YouStillAlive added 3 commits March 7, 2025 13:46

remove tautology

e9c172c

use LastPoolOwnerState

3bab7ee

fix codefactor

650cf62

Lomet reviewed Mar 10, 2025

View reviewed changes

YouStillAlive added 2 commits March 10, 2025 14:25

update compile settings

8efd679

update evmVersion

f348001

YouStillAlive and others added 6 commits March 11, 2025 14:26

version 4 - use params from provider, deacrease internals

9b2c91a

add delete for state

be0e1b3

compromise between setting to 0 and optimizing

e4a729f

return higher provider support

1d003d1

Merge pull request #432 from The-Poolz/issue-393-5

1869240

Issue 393 `version 5`

move _mintNewNFT function to dealProvider file

dd0bc29

YouStillAlive merged commit c5aa1e5 into master Mar 13, 2025
7 checks passed

YouStillAlive deleted the issue-393 branch March 13, 2025 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

immutable `withdraw` + `split` #428

immutable `withdraw` + `split` #428

YouStillAlive commented Mar 6, 2025

github-actions bot commented Mar 6, 2025 •

edited

Loading

github-actions bot commented Mar 7, 2025 •

edited

Loading

codecov bot commented Mar 7, 2025 •

edited

Loading

Lomet Mar 10, 2025

YouStillAlive commented Mar 11, 2025 •

edited

Loading

immutable withdraw + split #428

immutable withdraw + split #428

Conversation

YouStillAlive commented Mar 6, 2025

github-actions bot commented Mar 6, 2025 • edited Loading

Slither report

incorrect-equality

uninitialized-local

unused-return

calls-loop

timestamp

dead-code

naming-convention

immutable-states

github-actions bot commented Mar 7, 2025 • edited Loading

Methods

Deployments

Solidity and Network Config

codecov bot commented Mar 7, 2025 • edited Loading

Codecov Report

Lomet Mar 10, 2025

Choose a reason for hiding this comment

YouStillAlive commented Mar 11, 2025 • edited Loading

Gas tracking

immutable `withdraw` + `split` #428

immutable `withdraw` + `split` #428

github-actions bot commented Mar 6, 2025 •

edited

Loading

github-actions bot commented Mar 7, 2025 •

edited

Loading

codecov bot commented Mar 7, 2025 •

edited

Loading

YouStillAlive commented Mar 11, 2025 •

edited

Loading