Munchables - SovaSlava's results

Lines of code

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

Vulnerability details

Impact

A user can block another user's function call unlock() by calling the lockOnBehalf() even with a zero value, thereby prolonging the locking time.

Proof of Concept

There is 2 problems: user could not specify approved list of addresses, which allow call lockOnBehalf() and specify user's address. And second is that there is no minimum required amount of tokens/eth for lock. Attacker could frontrun user's call to unlock(), and just call function lockOnBehalf() and specify user's address and token.

Tools Used

Manual review

Recommended Mitigation Steps

Add approve mechanism, where user could specify whitelisted address, which could lock tokens on behalf the user. And add check if _quantity > minQuantity.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Due to incorrect verification of time duration, the user can reduce the locking time.

Proof of Concept

The unlockTime variable in setLockDuration() is updated as lastLockTime plus _duration, but _duration must be added to the current time, as in the check above. Example: User have: lastLockTime=100 and unlockTime = 1000. Block.timestamp is 800. User call setLockDuration(300)

if ( uint32(block.timestamp) + uint32(_duration) <
      lockedTokens[msg.sender][tokenContract].unlockTime
       //  800+300 > 1000. The verification will be passed  <---  
   ) {
      revert LockDurationReducedError();
     }

And new unlockTime will be updated as lastLockTime(100) + duration(300) = 100 + 300 = 400.

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

Previous value of unlockTime was 1000, now - 400.

Tools Used

Manual review

Recommended Mitigation Steps

- uint32 lastLockTime = lockedTokens[msg.sender][tokenContract].lastLockTime;
  lockedTokens[msg.sender][tokenContract].unlockTime =
-                   lastLockTime +
+                   block.timestamp +
                    uint32(_duration);

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

When user lock tokens, and execute calculation count of nfts, some value could be write into remainder value. But after unlockTime, user can call unlock() and get all tokens back, but remainder value does not delete, so in next call to lock() function, he could use less tokens, than it needed, because will be uses specified quantity and remainder value from previous lock.

Proof of Concept

Example: configuredToken.nftCost = 3; User call lock(10) -> quantity=10, remainder=1 (10%3=1)

function lock() {
   ....
   remainder = quantity % configuredToken.nftCost;
   numberNFTs = (quantity - remainder) / configuredToken.nftCost;
   ...
   lockedToken.remainder = remainder; // 1
   lockedToken.quantity += _quantity; // 3

When block.timestamp will be > unlockTime, user call unlock(10).

function unlock() {
   ...
   lockedToken.quantity -= _quantity;
   // But dont set 0 in remainder value.

User call lock(8)

function lock() {
   ...
   uint256 quantity = _quantity + lockedToken.remainder;  // 8 + 1= 9
   ...
   remainder = quantity % configuredToken.nftCost; // 9%3=0
   numberNFTs = (quantity - remainder) / configuredToken.nftCost; // (9-0) / 3=3

So, user use less tokens in second call of lock, than in first call.

Tools Used

Manual review

Recommended Mitigation Steps

   function unlock() {
   ...
   lockedToken.quantity -= _quantity;
+  lockedToken.remainder = 0;

Assessed type

Other