Munchables - AvantGard's results

Lines of code

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

Vulnerability details

Impact

Griefing Attack: Lack of access control in lockOnBehalf function allows malicious users to lock the tokens of other players by repeatedly extending the unlockTime.

Impact: High Likelihood : High

Proof of Concept

Contract : LockManager

As it stands

lockOnBehalf function allows any msg.sender to lock tokens on behalf of another address _onBehalfOf, which is the root cause of this attack.

The following example demonstrates the attack on a single user, executed one time. But note that, this attack could be performed repeatedly on multiple player accounts.

Bob : Player/ victim Alice : Attacker

Step 1:

Bob completes the registration and locks 5 WETH on May 1, 2024 ( block.timestamp = 1714521600)

vm.startPrank(Bob);

lockManagerContract.lock(address(weth), 5 ether);

vm.stopPrank();

Going by the minLockDuration of 30 days

lockedToken.unlockTime for weth is set to May 30, 2024 (block.timestamp = 1717027200)

playerSettings[_lockRecipient].lockDuration is set to minLockDuration = 30 days

Step 2:

Alice wants to keep Bob's funds locked in the contract to execute a griefing attack or gain any potential advantage in game.

On May 29, 2024 (block.timestamp = 1716940800)

Alice calls the lockOnBehalf function with dust amount of weth passing Bob's player account as the lockRecipient

vm.startPrank(Alice);

lockManagerContract.lockOnBehalf(
                    address(weth), 
                    1 wei, 
                    address(Bob)) 

vm.stopPrank();

Since playerSettings[_lockRecipient].lockDuration was set to 30 days. The following line extends Bob's unlockTime till June 28, 2024.

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

Step 3:

Bob attempts to withdraw his weth funds on May 31st

vm.startPrank(Bob);

vm.expectRevert();
lockManagerContract.unlock(address(weth), 5 ether);

vm.stopPrank();

and gets reverted with TokenStillLockedError due to the following check

if (lockedToken.unlockTime > uint32(block.timestamp))
revert TokenStillLockedError();

Any user is able to execute this attack repeatedly, keeping other players' funds locked indefintely.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

To prevent this, lockOnBehalf should require explicit consent from the lockRecipient.

One way to implement this is by introducing an approval mechanism where users must approve another address to lock tokens on their behalf.

1. Add a mapping to store approvals

mapping(address => mapping(address => bool)) public lockOnBehalfApproval;

2. Add a function to allow users to approve another address:

function approveLockOnBehalf(address _approved) external {
    lockOnBehalfApproval[msg.sender][_approved] = true;
    emit ApprovalForLockOnBehalf(msg.sender, _spender);
}

3. Modify lockOnBehalf to check if the msg.sender is approved

With these changes, users can control who is allowed to lock tokens on their behalf, mitigating the risk of unauthorized extensions of the lock period.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Likelihood : High Impact : High

The following is one of the main invariants mentioned in the protocol documentation.

https://github.com/code-423n4/2024-05-munchables/tree/main?tab=readme-ov-file#attack-ideas-where-to-focus-for-bugs

people cannot reduce lockup times that are previously set

However, any user is able to exploit the following math error in setLockDuration function to unlock locked funds earlier than intended by the protocol.

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

This breaks the above mentioned invariant.

Proof of Concept

Contract : lockManager

This vulnerability stems from the way unlockTime is calculated in the setLockDuration

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

The above calculation of unlockTime takes into account the lastLockTime instead of using the block.timestamp value, which is the root cause of this issue.

Lets demonstrate with an example

Step 1:

Bob locks 5 Ether of WETH tokens for the first time on 12 am, May 1, 2024 ( block.timestamp = 1714521600)

vm.startPrank(Bob);
lockManagerContract.lock ( address(weth), 5 ether )

_lockDuration is assigned a minLockDuration of 30 days. such that

lastLockTime = 12 am, May 1, 2024 ( block.timestamp = 1714521600) unlockTime = 12 am, May 30, 2024 (block.timestamp = 1717027200)

Step 2:

Bob waits till 1 am, May 16, 2024 ( block.timestamp = 1715821200)

Step 3 :

Bob calls setLockDuration at 1 am, May 16th 2024 with a _duration = 15 days, inorder to avoid the following revert

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

The unlockTime is calculated as

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

1714521600 + 15 days = 1715817600 (12 am , May 16, 2024 )

which is clearly in the past.

Step 4 :

Bob immediately calls unlock and withdraws the WETH tokens much earlier than the minimum lock duration intended by the protocol

vm.startPrank(Bob);
lockManagerContract.unlock(address(weth), 5 ether);

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

To prevent this exploit, the new unlockTime should be calculated as the sum of current block.timestamp and _duration

block.timestamp + uint32(_duration)

 if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
            // Ensure new unlock time is in the future and greater than current unlock time
            uint32 newUnlockTime = uint32(block.timestamp) + uint32(_duration);

            lockedTokens[msg.sender][tokenContract].unlockTime = newUnlockTime;
            
            if (newUnlockTime < lockedTokens[msg.sender][tokenContract].unlockTime) {
                revert LockDurationReducedError();
            }
        }
        

As an additional measure, check should be made after the update to ensure that the updated unlockTime remains larger than the previous unlockTime

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Likelihood : High Impact : High

The unlock function is currently missing the logic to adjust lockedToken.remainder correctly when unlocking tokens in some edge cases.

This results in an unfair benefit where an user gains an additional NFT without locking the required amount.

Proof of Concept

Consider

• cost of single NFT = 1000 USDB
• minimum lockup duration = 5 days

Step 1

Bob locks 1900 USD, 
gets 1 NFT. 


lockedToken.remainder = 900 USDB;
lockedToken.quantity = 1900 USDB;	

Step 2

Bob Waits 5 days (minimum lockup duration)

Step 3

Bob calls `unlock` with _quantity of 1900 USDB.
remainder is still at 900 USDB

Adjust the locked quantity:

lockedToken.quantity -= _quantity; // lockedToken.quantity -= 1900 = 0	

However note that,
`lockedToken.remainder` is not adjusted and remains 900

Step 4

 Bob calls `lock` again with `_quantity` of 100 USDB.

 `quantity`  = 100 + 900 (previous remainder) = 1000 USDB
 remainder  = 0

 numberNFTs = (quantity - remainder) / configuredToken.nftCost
	    = (1000 - 0 ) / 1000 
	    = 1
 
 Thus Bob is able to acquire the subsequent NFT by locking only 100 USDB

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

To prevent this issue, the unlock function should be corrected to adjust the lockedToken.remainder appropriately when tokens are unlocked.

Assessed type

Math