Munchables - joaovwfreire's results

Lines of code

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

Vulnerability details

Description

The lock private function increases the amount of certain token locked by an address at the LockManager contract by transferring a corresponding amount of tokens into it. As a side-effect, it increases the unlockTime by the current lock's lock duration. This opens up the possibility of attackers to lock someone's tokens forever at the contract.

The reason for that is the lock private function has two entry points: via the lock and via the lockOnBehalf functions. The latter allows anyone to target any address as the tokenRecipient - as it offers no access control from callers to lock recipients. If any address can be targeted, anyone can increase a lock's unlock time, as can be seen in the following code snippet:

function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
...		
		lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);
...
}

Impact

Malicious actors can freely increase the unlock time of users' locks. If the gas cost is low at any point, then maintaining one's lock forever is relatively cheap.

Tools Used

Manual review

Recommended Mitigation Steps

Make sure to create some sort of access control for locking on behalf of someone, as it has the side-effect of increasing the unlock time. An interesting possibility would be to create a mapping of allowed addresses to lock on behalf of an address then check if the lock on behalf caller is allowed to do such an action to its lock recipient. This access control logic will mitigate the aforementioned attack vector.

Assessed type

DoS

Lines of code

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

Vulnerability details

Description

The setLockDuration function does a couple of checks to ensure the new lock duration does not decrease the previously existing one. However, its current logic allows this requirement to be bypassed. This happens due to the following reasons: The function does not check if the uint256 duration argument is smaller than the minimum duration at the contract.

The LockDurationReducedError only checks for the current timestamp + duration in comparision to the last unlockTime:

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

The new unlockTime is not based on the previously used, but on the lastLockTime - that is, the time the previous lock was created.

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

This enables malicious parties to reduce the lock duration by doing the following steps:

1. Create a lock. Say the lock starts at timestamp 1000 and it has a duration of 1000 seconds. This lock will have a lastLockTime of 1000 and unlockTime of 2000.
2. Wait 500 seconds. Call setLockDuration with the new duration as 501 seconds.
   1. The call will not revert as the LockDurationReducedError won't be executed: the current block.timestamp + the new duration results in a timestamp that is equal to 2001, bigger than the unlockTime (2000).
   2. Unlock time will become lastLockTime + new duration. Since lastLockTime is 1000, the new unlockTime is 1501.
3. Wait 1 second. Call unlock and recover the funds after almost half of the lock's duration. Notice: lock durations and timestamps here provided are for the purpose of exemplifying and the logic does apply to different values.

Impact

Locks are able to be manipulated to last half of its original duration.

Tools Used

Manual review

Recommended Mitigation Steps

Make sure to review the LockDurationReducedError checks. If the intention is to never allow lock duration reductions, the check should evaluate duration + last lock time instead of block.timestamp + duration, as can be seen in the code snippet bellow:

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

This will ensure the new unlock time fits the bounds defined by the initial lock timestamp.

Assessed type

Timing

Lines of code

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

Vulnerability details

Description

When dealing with usd price proposals, price feeds ought to have two options: vote to approve or to disapprove a proposed price. This is not enforced on its entirety at the LockManager contract, however. The disapproveUSDPrice function properly enforces that by checking whether the price feed hasn't approved nor disapproved a proposed price before accounting the disapprove vote:

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

The approveUSDPrice doesn't enforce the same checks. It only verifies the price feed hasn't approved the price priorly:

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

Impact

The current price approval mechanism is not fair on all price fees. This issue allows privilege escalation as a price feed is able to vote twice.

Allowing one voter to vote on both approval and disapproval exceeds the intended permissions by creating the possibility of voting ties: the system has 5 price feeds so that ties are impossible, but this privilege escalation allows both for and against positions to have 3 or more votes. This enables an unexpected logic for price approvals - price feeds will have incentives to vote as soon as possible.

Tools Used

Manual review

Recommended Mitigation Steps

Add the following check to approveUSDPrice:

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

Assessed type

Other

Lines of code

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

Vulnerability details

Description

The lock private function can be executed from the lock and from the lockOnBehalf public functions. When called by the migrationManager role, the lock private function does a wrong state management on a locked token's remainder. Let's analyze it in-depth: The remainder uint256 variable is initialized as equal to zero:

uint256 remainder;

Then the function proceeds to update this variable only if the msg.sender is not the contract's migrationManager:

if (msg.sender != address(migrationManager)) {
                // calculate number of nfts
                remainder = quantity % configuredToken.nftCost;
...

Finally it takes the current remainder value and updates the remainder at the lockedToken storage mapping:

lockedToken.remainder = remainder;

The issue with that is that if we don't ever reach the if block limited by the migrationManager caller, we put a zero-valued remainder at the storage in lockedToken.remainder. This removes any remainder the user had left.

Impact

Users that have their locks updated by the migrationManager lose their locked token's remainder.

Tools Used

Manual review

Recommended Mitigation Steps

Make sure to avoid updating the remainder storage variable inside a lockedToken if the currently calculated value is not different than the previously held. An interesting way would to keep the remainder value would be through an else block:

if (msg.sender != address(migrationManager)) {
...
}
else {
	remainder = lockedToken.remainder;
}

This should suffice to ensure the remainder is not lost in this case.

Assessed type

Context