Munchables - araj's results

Lines of code

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

Vulnerability details

Vulnerability details

When a user lock his tokens then his unlockTime increases by _lockDuration in LockManager::_lock()

 function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
       ...
     @>   uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

     @>   if (_lockDuration == 0) {
            _lockDuration = lockdrop.minLockDuration;
        }
       ...
     @>   lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

       ...
    }

Now, any user can deposit on behalf of other user by calling LockManager::lockOnBehalf() which will again increase his unlockTime as we've seen above

A malicious user can take advantage of this function to grief honest user by frontrunning their unlock(), which will make their unlock revert due to below check

 function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
    ...
     @>   if (lockedToken.unlockTime > uint32(block.timestamp))
            revert TokenStillLockedError();
      ...
    }

Important:- This can be done with 0(zero) token as there is no check for min amount

Impact

Honest user will be DOS

Proof of Concept

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L410C8-L411C44 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L382C1-L384C35

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure min lock amount or avoid locking tokens onBehalf of other users

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L265

Vulnerability details

Vulnerability details

When a user set his lockDuration using setLockDuration(), it ensures that unlockTime is not reduced but the problem is instead of adding block.timestamp to _duration in unlockTime, lastLockTime is added, which reduces the unlockTime

  function setLockDuration(uint256 _duration) external {
       ...
           uint32 lastLockTime = lockedTokens[msg.sender][tokenContract].lastLockTime;
         @>  lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration);
      ...
    }

//How this works(for simplicity taking time is hrs)

1. Suppose user locked at 1PM and minLockDuration = 6hrs then lastLockTime = 1PM & unlockTime = 7PM(1PM + 6hrs)
2. His current lockDuration = 6hrs(ie minLockDuration, that get set in _lock())
3. At 5PM, user decided to change his lockDuration = 3hrs, so block.timestamp + duration = 5PM + 3hrs = 8PM, ie check pass because 8PM > 7PM(unlockTime)
4. Now, according to above code unlockTime = lastLockTime + duration = 1PM + 3hrs = 4PM, which reduces the unlockTime from 7PM to 4PM

Impact

unlockTime can be reduced. As a result, user can unlock early

Proof of Concept

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L263C17-L267C39

Tools Used

Manual Review

Recommended Mitigation Steps

Replace lastLockTime with block.timestamp

Assessed type

Error

Lines of code

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

Vulnerability details

Vulnerability details

Other priceFeeds can approve/disapprove the price proposed by a priceFeed and to prevent a priceFeed from voting twice, there is a check in disapproveUSDPrice()

 function disapproveUSDPrice(
        uint256 _price
    )
     ...
 
     @>   if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
     @>  if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();
    ...
    }

In above code, there are 2 checks which ensures priceFeed has neither approved or disapproved the price.

But the problem is there is only 1 check in approveUSDPrice(), which only checks for priceFeed has approved but not for disapprove, allowing a priceFeed to disapprove(via disapproveUSDPrice) first and then approve(via approveUSDPrice) the price, giving an option to approve & disapprove simultaneously

  function approveUSDPrice(
        uint256 _price
    )
    ...
      @>  if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
    ...
    }

Impact

PriceFeed can approve & disapprove simultaneously

Proof of Concept

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L177C4-L207C6 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L210C4-L242C6

Tools Used

Manual Review

Recommended Mitigation Steps

Add this line in approveUSDPrice()

+ if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
+            revert ProposalAlreadyDisapprovedError();

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L401

Vulnerability details

Vulnerability details

When a user locks his assets then remainder & quantity is saved in user's lockedToken.remainder and lockedToken.quantity. Also remainder is added to quantity when user locks his assets next time

function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
     ...
     @>   uint256 quantity = _quantity + lockedToken.remainder;
        uint256 remainder;
     ...
      @>  lockedToken.remainder = remainder;
      @> lockedToken.quantity += _quantity;
     ...
    }

Now the problem is, when a user unlock() his assets then only quantity is reduced, but not remainder

 function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
     ...
        lockedToken.quantity -= _quantity;
     ...
    }

As a result, when user will lock again then the left remainder will be utilized again //How this works

1. Suppose nftCost = 5 ether and user lock 9 ether then his remainder = 4 ether & quantity = 9 ether
2. User unlock his 9 ether, which reduced his quantity to 0(zero)
3. User again lock 1 ether, then left remainder will be added to quantity before buying nft ie 1(lock) + 4(remainder) = 5 ether, user will get 1 NFT at 1 ether price

     // calculate number of nfts
     remainder = quantity % configuredToken.nftCost;
     numberNFTs = (quantity - remainder) / configuredToken.nftCost;

Impact

User will get NFT at much cheaper price, which is loss for protocol

Proof of Concept

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L401C1-L427C6 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L344 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L380

Tools Used

Manual Review

Recommended Mitigation Steps

Subtract the remainder in unlock()

Assessed type

Error