Create vlPUFFER by bxmmm1 · Pull Request #3 · PufferFinance/puffer-locker

bxmmm1 · 2025-05-07T11:31:49Z

No description provided.

src/vlPUFFER.sol

foundry.toml

src/vlPUFFER.sol

eladiosch · 2025-05-08T11:54:35Z

src/vlPUFFER.sol

+    // 1% in basis points
+    uint256 internal constant _KICKER_FEE_BPS = 100;
+    // 10000 in basis points
+    uint256 internal constant _KICKER_FEE_DENOMINATOR = 10000;


When writing numbers in basis points I like to write them like this 100_00 for clarity. I don't really mind, just a suggestion

you mean 10_000 ? :D

No, no. I mean 100_00. And 1_00 for _KICKER_FEE_BPS. It's like indicating 100.00% or 1.00%. Since it's basis points the last 2 positions are decimals. 10_000 is even more confusing :)

I've added a configuration so that it automatically formats thousands when you do forge fmt

number_underscore = "thousands"

https://book.getfoundry.sh/reference/config/formatter?highlight=number_underscore%20%3D%20%22thousands%22#number_underscore

That's a good config, but for this case: meh. We should add a // forgefmt: disable-next-line because that's a confusing way to write basis points

src/vlPUFFER.sol

eladiosch · 2025-05-08T15:41:26Z

src/vlPUFFER.sol

+        uint256 pufferAmount,
+        uint256 pufferAmountAdded,
+        uint256 unlockTime,
+        uint256 vlPUFFERAmount


Since we're showing the increase in pufferAmount, should we do the same in vlPUFFERAmount? Not sure about this since it can be calculated from the data of the event + the timestamp

Or maybe remove it completely?, There are transfer events in the PUFFER token, we can figure it out if needed, also it can be read from the calldata. Maybe we don't need it in the event at all pufferAmountAdded wyt?

Hmm from a BE it can be calculated without problem, but for manual inspection of a tx in the explorer the info in the event might be useful. I'd rather still have it but I don't really mind if it's removed. Your call

eladiosch · 2025-05-08T15:45:44Z

src/vlPUFFER.sol

+     * @notice Withdraw the tokens
+     * @param recipient Address to receive the PUFFER tokens
+     */
+    function withdraw(address recipient) external {


Do we want to implement an emergencyWithdraw function or nah? As I understand it, it would be just like this but ignoring the TokensLocked check and only whenPaused

We should have a discussion on this. Without additional context, its really useless. We need to plan for the future and figure out if ti makes sense to add it.

src/vlPUFFER.sol

tdenisenko · 2025-05-12T09:54:48Z

src/vlPUFFER.sol

+    function _calculateUnlockTimeAndMultiplier(uint256 unlockTime)
+        internal
+        view
+        returns (uint256 roundedUnlockTime, uint256 multiplier)
+    {
+        multiplier = ((unlockTime - block.timestamp) / _LOCK_TIME_MULTIPLIER);
+        // Round down the unlockTime to the nearest 30-day multiplier
+        roundedUnlockTime = block.timestamp + (multiplier * _LOCK_TIME_MULTIPLIER);
+    }


So if user tries to lock for 59 days he will get 30 days instead. This is unintuitive and unexpected I'd say. I'd suggest instead of unlock time in create lock we can ask for the multiplier directly (between 1 and 24). You can call it multiplier or months

Correct. The frontend will always use a bigger timestamp to achieve the desired result.

Scenario:
User is on our frontend, and wants to lock for 1 year = x12 multiplier. When the frontend prepares the transaction, the timestamp will be:

unlockTImestamp = now + 1 year in seconds + 30 days in seconds (if a user uses really low gas price, and it takes a few days for the transaction to be included). This will round down in the smart contract, and the user will get the desired multiplier.

It can be refactored to use multiplier instead of the unlockTime. We should also have a sync with the business team before auditing the code.

ksatyarth2 · 2025-05-15T12:53:42Z

src/vlPUFFER.sol

+    function createLockWithPermit(uint256 value, uint256 unlockTime, uint256 deadline, uint8 v, bytes32 r, bytes32 s)
+        external
+    {
+        IERC20Permit(address(PUFFER)).permit(msg.sender, address(this), value, deadline, v, r, s);


where is the transfer of PUFFER after permit? bug?

ksatyarth2 · 2025-05-15T12:58:08Z

src/vlPUFFER.sol

+     * @param r First 32 bytes of the signature
+     * @param s Second 32 bytes of the signature
+     */
+    function createLockWithPermit(uint256 value, uint256 unlockTime, uint256 deadline, uint8 v, bytes32 r, bytes32 s)


i would suggest to use this pattern for permit as we are already following this pattern on our frontend:
Permit calldata permitData
and then:
permitData.v, ....

and also use try() catch() for permit so that we don't get internal error on etherscan.

ksatyarth2 · 2025-05-15T13:01:50Z

src/vlPUFFER.sol

+        multiplier = ((unlockTime - block.timestamp) / _LOCK_TIME_MULTIPLIER);
+        // Round down the unlockTime to the nearest 30-day multiplier
+        roundedUnlockTime = block.timestamp + (multiplier * _LOCK_TIME_MULTIPLIER);


auditors might raise issue of underflow/overflow here. and suggest to use SafeMath and checks

i did cast block timestamp to uint256, to avoid this issue. good catch

tdenisenko · 2025-05-15T18:06:07Z

src/vlPUFFER.sol

-    uint256 internal constant _KICKER_FEE_DENOMINATOR = 10_000;
+    // 100% in basis points
+    // forgefmt: disable-next-line
+    uint256 internal constant _KICKER_FEE_DENOMINATOR = 100_00;


100_00 ? it should be 10_000

actually we don't even need this as a constant. the term BPS already implies 10,000

true, but since it is not upgradeable contract, and we are hardcoding it. it might as well be a readable. this suggestion was made by @eladiosch because 10k bps = 100 %, so i just followed up on it

tdenisenko · 2025-05-15T18:12:32Z

src/vlPUFFER.sol

+        // User may deposit more tokens to old lock, or extend the lock
+        require(unlockTime >= lockInfo.unlockTime, UnlockTimeMustBeGreaterOrEqualThanLock());
+        require(lockInfo.pufferAmount > 0, LockDoesNotExist());
+
+        (uint256 roundedUnlockTime, uint256 multiplier) = _calculateUnlockTimeAndMultiplier(unlockTime);


what if rounded unlock time is the same as unlock time?

tdenisenko · 2025-05-15T18:13:35Z

src/vlPUFFER.sol

+        require(lockInfo.pufferAmount > 0, LockDoesNotExist());
+        require(lockInfo.unlockTime <= block.timestamp, TokensLocked());
+


inconsistent usage of require + revert everywhere, you should pick one and use that only

IMO, we don't need to stay consistent with require vs revert. In some cases, it is more readable to have it in require, while on the others, if () revert makes a little more sense.

tdenisenko · 2025-05-15T18:15:29Z

src/vlPUFFER.sol

+            PUFFER.safeTransferFrom(msg.sender, address(this), amount);
+        }
+
+        LockInfo memory lockInfo = lockInfos[msg.sender];


No need to copy this to memory, you can make it a storage variable and also use this variable for the assignment on line 255

tdenisenko · 2025-05-15T18:16:30Z

src/vlPUFFER.sol

+            _burn(user, vlPUFFERAmount);
+
+            // Send the rest of the PUFFER tokens to the user
+            PUFFER.safeTransfer(user, pufferAmount - kickerFee);


what if user can't recover ERC20

The user was the one who initially locked ERC20 tokens. This makes me think, if a user can use 7702 to revert this transaction, making it impossible to get kicked. I'll research this topic a little more, before we finalize the contract.

I see how usign 7702 user could prevent native token from being received. However, this is a ERC20 transfer. Unless our Puffer token tried to call an onTokensReceived hook like ERC777, I don't think user can make the kick tx revert. The only case I can think regarding Tim's comment is the the user is a smart contract without a function to recover ERC20. And about that case, we should just inform about the kick flow so if a contract wants to use this protocol, they should implement a way to recover ERC20

Yeah, I wrote a test to verify that.

tdenisenko · 2025-05-15T18:18:43Z

src/vlPUFFER.sol

+        roundedUnlockTime = uint256(block.timestamp) + (multiplier * _LOCK_TIME_MULTIPLIER);
+    }
+
+    function _createLock(uint256 amount, uint256 unlockTime) internal onlyValidLockDuration(unlockTime) whenNotPaused {


getting multiplier as param directly instead of unlock time would make it more readable since there would be no uncertainty or mental calculations

tdenisenko · 2025-05-15T18:20:31Z

src/vlPUFFER.sol

-    uint256 internal constant _KICKER_FEE_DENOMINATOR = 10_000;
+    // 100% in basis points
+    // forgefmt: disable-next-line
+    uint256 internal constant _KICKER_FEE_DENOMINATOR = 100_00;


actually we don't even need this as a constant. the term BPS already implies 10,000

ksatyarth2

All LGTM now! Just a suggestion to add fuzz tests considering our last audit

bxmmm1 · 2025-05-20T10:49:50Z

@tdenisenko Pls review

bxmmm1 added 2 commits May 7, 2025 13:31

add vlPUFFER contract

cb07bf6

change casing

b416316

bxmmm1 requested review from eladiosch, ksatyarth2 and tdenisenko May 7, 2025 13:18

bxmmm1 self-assigned this May 7, 2025

bxmmm1 changed the base branch from dev to main May 7, 2025 13:19

bxmmm1 changed the base branch from main to dev May 7, 2025 13:19

bxmmm1 added 4 commits May 7, 2025 16:14

update readme

7611dde

update kickUser to kickUsers

91d919d

add more tests

8d0385e

update snapshot

66e804b

eladiosch reviewed May 8, 2025

View reviewed changes

src/vlPUFFER.sol Outdated Show resolved Hide resolved

eladiosch reviewed May 8, 2025

View reviewed changes

src/vlPUFFER.sol Outdated Show resolved Hide resolved

write delegation test

71b129b

bxmmm1 commented May 8, 2025

View reviewed changes

src/vlPUFFER.sol Outdated Show resolved Hide resolved

bxmmm1 added 3 commits May 8, 2025 11:22

remove permit, token is not transferrable

ab46b21

update multiplier and unlock time

441a430

update gas snapshot

163c02e

eladiosch reviewed May 8, 2025

View reviewed changes

src/vlPUFFER.sol Outdated Show resolved Hide resolved

eladiosch reviewed May 8, 2025

View reviewed changes

src/vlPUFFER.sol Outdated Show resolved Hide resolved

eladiosch reviewed May 8, 2025

View reviewed changes

foundry.toml Show resolved Hide resolved

eladiosch reviewed May 8, 2025

View reviewed changes

src/vlPUFFER.sol Outdated Show resolved Hide resolved

eladiosch reviewed May 8, 2025

View reviewed changes

src/vlPUFFER.sol Outdated Show resolved Hide resolved

eladiosch reviewed May 8, 2025

View reviewed changes

address pr comments

9c955ec

eladiosch reviewed May 8, 2025

View reviewed changes

src/vlPUFFER.sol Show resolved Hide resolved

bxmmm1 added 3 commits May 8, 2025 14:40

update config and fmt

20734c2

code style

45c31e0

update comment

67eb0fa

eladiosch reviewed May 8, 2025

View reviewed changes

src/vlPUFFER.sol Outdated Show resolved Hide resolved

eladiosch reviewed May 8, 2025

View reviewed changes

src/vlPUFFER.sol Outdated Show resolved Hide resolved

eladiosch reviewed May 8, 2025

View reviewed changes

src/vlPUFFER.sol Show resolved Hide resolved

bxmmm1 added 2 commits May 9, 2025 09:49

minor changes

ee7fb2e

gas snapshot

5a391dc

tdenisenko reviewed May 12, 2025

View reviewed changes

ksatyarth2 reviewed May 15, 2025

View reviewed changes

bxmmm1 added 3 commits May 15, 2025 18:40

update vlPUFFER

4bce0f5

update formatting

488c9a1

update readme

c4aa253

bxmmm1 requested review from eladiosch, ksatyarth2 and tdenisenko May 15, 2025 17:11

bxmmm1 changed the title ~~add vlPUFFER locker~~ Create vlPUFFER May 15, 2025

tdenisenko reviewed May 15, 2025

View reviewed changes

tdenisenko suggested changes May 15, 2025

View reviewed changes

ksatyarth2 reviewed May 16, 2025

View reviewed changes

ksatyarth2 approved these changes May 16, 2025

View reviewed changes

update locker to use multiplier instead of unlockTime

3894d4b

eladiosch approved these changes May 16, 2025

View reviewed changes

bxmmm1 added 2 commits May 19, 2025 08:28

updage snasphot

08acc8e

snapshot 7702 test

459575f

bxmmm1 requested a review from tdenisenko May 19, 2025 07:54

tdenisenko approved these changes May 20, 2025

View reviewed changes

bxmmm1 merged commit c64d833 into dev May 20, 2025
2 checks passed

		require(lockInfo.pufferAmount > 0, LockDoesNotExist());
		require(lockInfo.unlockTime <= block.timestamp, TokensLocked());

Comments

Conversation

bxmmm1 commented May 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eladiosch May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksatyarth2 left a comment

Choose a reason for hiding this comment

Uh oh!

bxmmm1 commented May 20, 2025

eladiosch May 8, 2025 •

edited

Loading