Munchables - bigtone's results

Lines of code

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

Vulnerability details

Impact

The lockOnBehalf function allows locking tokens on behalf of another player, but it lacks validation to ensure that msg.sender has permission to act on behalf of the player.

Proof of Concept

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

File: src/managers/LockManager.sol#L275
    function lockOnBehalf(
        address _tokenContract,
        uint256 _quantity,
        address _onBehalfOf
    )
        external
        payable
        notPaused
        onlyActiveToken(_tokenContract)
        onlyConfiguredToken(_tokenContract)
        nonReentrant
    {
        address tokenOwner = msg.sender;
        address lockRecipient = msg.sender;
        if (_onBehalfOf != address(0)) {
            lockRecipient = _onBehalfOf;
        }

        _lock(_tokenContract, _quantity, tokenOwner, lockRecipient);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Implement proper authorization checks in the lockOnBehalf function to ensure that msg.sender has explicit permission to act on behalf of the player. This can be achieved by incorporating an approval mechanism.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Users can increase the unlockTime by using the setLockDuration function. Although there is validation to prevent reducing the lock duration, it can be bypassed due to the incorrect setting of unlockTime.

Proof of Concept

After one block, lastLockTime becomes less than block.timestamp. Users can call setLockDuration with playerSettings' duration minus 1, thereby bypassing the LockDurationReducedError.

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

File: src/managers/LockManager.sol#L245
    function setLockDuration(uint256 _duration) external notPaused {
        ...
            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 // @audit block.timestamp > .lastLockTime
                ) {
                    revert LockDurationReducedError();
                }

                uint32 lastLockTime = lockedTokens[msg.sender][tokenContract]
                    .lastLockTime;
                lockedTokens[msg.sender][tokenContract].unlockTime =
                    lastLockTime +
                    uint32(_duration); // @audit unlockTime should be block.timestamp + _duration
            }
        ...
    }
File: test.t.sol
    function test_SpeedRun() public {
        uint256 lockAmount = 100e18;
        deployContracts();
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        lm.lock{value: lockAmount}(address(0), lockAmount);

        ILockManager.LockedTokenWithMetadata memory m = lm.getLocked(address(this))[0];
        assertEq(m.lockedToken.unlockTime, 86401); // unlock time is 1 day + 1 second

        vm.warp(block.timestamp + 1);

        for (uint i; i <= 1 days; i++) {
            lm.setLockDuration(1 days - i);
        }

        m = lm.getLocked(address(this))[0];
        assertEq(m.lockedToken.unlockTime, block.timestamp - 1); // unlock time is block.timestamp - 1 ...

        lm.unlock(address(0x0), lockAmount); // unlock all tokens
    }

Tools Used

Manual review

Recommended Mitigation Steps

It's recommended to update the unlockTime as follows :

File: src/managers/LockManager.sol#L245
    function setLockDuration(uint256 _duration) external notPaused {
        ...
            if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
+                uint32 lastLockTime = lockedTokens[msg.sender][tokenContract]
+                    .lastLockTime;
                // check they are not setting lock time before current unlocktime
                if (
-                    uint32(block.timestamp) + uint32(_duration) <
+                    lastLockTime + 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);
    }

Assessed type

DoS

Judge has assessed an item in Issue #550 as 2 risk. The relevant finding follows:

L4. A feeder who has already disapproved the USD price can approve it in the same proposal.

Impact

There are no checks to ensure that msg.sender has not already disapproved the price in the approveUSDPrice function. This means a feeder who has previously disapproved the same price can also approve it.

Proof of Concept

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

File: src/managers/LockManager.sol#L177
    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) // @audit missed disapprovals checks
            revert ProposerCannotApproveError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();
        ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

It is advised to include validation to check if it has already been disapproved.

File: src/managers/LockManager.sol#L177
    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();
        ...
    }

L1. Using uint32 for block.timestamp is generally not sufficient for representing block timestamps

Impact

This range covers timestamps up to around the year 2106.

Proof of Concept

File: src/interfaces/ILockManager.sol#L61
    struct USDUpdateProposal {
        uint32 proposedDate; // @audit around year 2106?
        address proposer;
        address[] contracts;
        uint256 proposedPrice;
        mapping(address => uint32) approvals;
        mapping(address => uint32) disapprovals;
        uint8 approvalsCount;
        uint8 disapprovalsCount;
    }
File: src/managers/LockManager.sol#L104
    uint32(block.timestamp)
File: src/managers/LockManager.sol#L166
    usdUpdateProposal.proposedDate = uint32(block.timestamp);
File: src/managers/LockManager.sol#L249
    playerSettings[msg.sender].lockDuration = uint32(_duration);
File: src/managers/LockManager.sol#L257
    uint32(block.timestamp) + uint32(_duration) <
File: src/managers/LockManager.sol#L267
    uint32(_duration);
File: src/managers/LockManager.sol#L353
    lockdrop.start <= uint32(block.timestamp) &&
File: src/managers/LockManager.sol#L354
    lockdrop.end >= uint32(block.timestamp)
File: src/managers/LockManager.sol#L359
    uint32(configStorage.getUint(StorageKey.MaxLockDuration))
File: src/managers/LockManager.sol#L381
    lockedToken.lastLockTime = uint32(block.timestamp);
File: src/managers/LockManager.sol#L383
    uint32(block.timestamp) +
File: src/managers/LockManager.sol#L384
    uint32(_lockDuration);
File: src/managers/LockManager.sol#L410
    if (lockedToken.unlockTime > uint32(block.timestamp))

Tools Used

Manual review

Recommended Mitigation Steps

It's recommended to use uint48 for timestamps.

L2. If APPROVE_THRESHOLD is 1, the _execUSDPriceUpdate() function should be invoked in the proposeUSDPrice function.

Impact

APPROVE_THRESHOLD can be set as 1, so it's needed to check if approvalsCount is greater than APPROVE_THRESHOLD in the proposedUSDPrice function.

Proof of Concept

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

File: src/managers/LockManager.sol#L171
    function proposeUSDPrice(
        uint256 _price,
        address[] calldata _contracts
    )
        external
        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )
    {
        ...
        usdUpdateProposal.approvalsCount++;

        emit ProposedUSDPrice(msg.sender, _price);
    }

Tools Used

Manual review

Recommended Mitigation Steps

It's recommended to add the validation for thresholds.

File: src/managers/LockManager.sol#L171
    function proposeUSDPrice(
        uint256 _price,
        address[] calldata _contracts
    ) {
        ...
        usdUpdateProposal.approvalsCount++;
+        if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
+            _execUSDPriceUpdate();
+        }

        emit ProposedUSDPrice(msg.sender, _price);
    }

L3. There are missing checks for the _approve and _disapprove thresholds.

Impact

The thresholds for _approve and _disapprove should be validated.

Proof of Concept

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

File: src/managers/LockManager.sol#L135
    function setUSDThresholds(
        uint8 _approve,
        uint8 _disapprove
    ) external onlyAdmin {
        if (usdUpdateProposal.proposer != address(0))
            revert ProposalInProgressError();
        APPROVE_THRESHOLD = _approve; //@audit should be >1
        DISAPPROVE_THRESHOLD = _disapprove; // should be >=1?

        emit USDThresholdUpdated(_approve, _disapprove);
    }

Tools Used

Manual review

Recommended Mitigation Steps

It's recommended to add the validation for thresholds.

File: src/managers/LockManager.sol#L135
    function setUSDThresholds(
        uint8 _approve,
        uint8 _disapprove
    ) external onlyAdmin {
        if (usdUpdateProposal.proposer != address(0))
            revert ProposalInProgressError();
+        require(_approve > 1);
+        require(_disapprove >= 1);
        APPROVE_THRESHOLD = _approve;
        DISAPPROVE_THRESHOLD = _disapprove;

        emit USDThresholdUpdated(_approve, _disapprove);
    }

L4. A feeder who has already disapproved the USD price can approve it in the same proposal.

Impact

There are no checks to ensure that msg.sender has not already disapproved the price in the approveUSDPrice function. This means a feeder who has previously disapproved the same price can also approve it.

Proof of Concept

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

File: src/managers/LockManager.sol#L177
    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) // @audit missed disapprovals checks
            revert ProposerCannotApproveError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();
        ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

It is advised to include validation to check if it has already been disapproved.

File: src/managers/LockManager.sol#L177
    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();
        ...
    }

L5. Proposer can disapprove the usdprice in the same proposal.

Impact

There are no checks to ensure that msg.sender is not the proposer in the disapproveUSDPrice function. This means the proposer can also disapprove the proposal.

Proof of Concept

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

File: src/managers/LockManager.sol#L210
    function disapproveUSDPrice(
        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.approvals[msg.sender] == _usdProposalId) // @audit missed proposer is msg.sender?
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();
        ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended to add validation to ensure that the caller is the proposer.

File: src/managers/LockManager.sol#L177
    function disapproveUSDPrice(
        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 ProposerCannotDisapproveError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();
        ...
    }

L6. Missing the _price validation in the proposedUSDPrice

File: src/managers/LockManager.sol#L171
159:     if (_contracts.length == 0) revert ProposalInvalidContractsError();
+        if (_price == 0) revert USDPriceInvalidError();

NC1. Event missing indexed field

Index event fields make the field more quickly accessible to off-chain tools that parse events. This is especially useful when it comes to filtering based on an address. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Where applicable, each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three applicable fields, all of the applicable fields should be indexed.

File: src/interfaces/ILockManager.sol
159:     event TokenConfigured(address _tokenContract, ConfiguredToken _tokenData); 
168:     event ProposedUSDPrice(address _proposer, uint256 _price);
172:     event ApprovedUSDPrice(address _approver);
176:     event DisapprovedUSDPrice(address _disapprover);
225:     event USDPriceUpdated(address _tokenContract, uint256 _newPrice);

NC2. Some errors are not used

Remove the unused error types.

File: src/interfaces/ILockManager.sol
228:     error OnlyAccountManagerError();
245:     error USDPriceInvalidError();
293:     error InvalidTokenContractError();

NC3. Some events are not used

Remove the unused event types.

File: src/interfaces/ILockManager.sol
220:     event DiscountFactorUpdated(uint256 discountFactor);

NC4. If LockManager does not handle receiving the native token, there is no need to declare the receive() function.

File: src/interfaces/ILockManager.sol
93:     receive() external payable {
94:        revert LockManagerRefuseETHError();
95:    }