Munchables - leegh's results

Lines of code

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

Vulnerability details

Impact

Users can reduce their lockup times, which is not expected by Munchables. The impacts are:

1. Users can unlock their locked tokens back earlier.
2. Users can harvest earlier than expected.

Proof of Concept

In function setLockDuration, before a new lock duration is set, the resulted new unlock time is requried not to be earlier than current unlock time (L257). This is guaranteed by reverting the tx if uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime (L256-L261). If that condition is met, the new unlock time is then updated. However, the new unlock time is set as lastLockTime + uint32(_duration) (L265-L267). The updating is wrong, as the resulted unlock time could be earlier than current unlock time.

The reason for the problem is the time used for the new unlock time checking is block.timestamp, while the time used for updating new unlock time is lastLockTime, and block.timestamp >= lastLockTime always holds.

Let's look at an example:

1. At begining, lastLockTime = day 0, duration = 30 days, unlockTime = day 30
2. User sets lock duration at day 10 (i.e. block.timestamp = day 10), and the new duration is 15 days (i.e. _duration = 25 days). The checking in L256-L261 passes as blocl.timestamp + _duration = day 10 + 25 days = day 35 > currentUnlockTime = day 30. Then the unlock time will be updated with lastLockTime + _duration = day 0 + 25 days = day 25 < currentUnlockTime = day 30. The unlock time is reduced.

245:    function setLockDuration(uint256 _duration) external notPaused {
246:        if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
247:            revert MaximumLockDurationError();
248:
249:        playerSettings[msg.sender].lockDuration = uint32(_duration);
250:        // update any existing lock
251:        uint256 configuredTokensLength = configuredTokenContracts.length;
252:        for (uint256 i; i < configuredTokensLength; i++) {
253:            address tokenContract = configuredTokenContracts[i];
254:            if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
255:                // check they are not setting lock time before current unlocktime
256:                if (
257:@>                  uint32(block.timestamp) + uint32(_duration) <
258:                    lockedTokens[msg.sender][tokenContract].unlockTime
259:                ) {
260:                    revert LockDurationReducedError();
261:                }
262:
263:                uint32 lastLockTime = lockedTokens[msg.sender][tokenContract]
264:                    .lastLockTime;
265:@>              lockedTokens[msg.sender][tokenContract].unlockTime =
266:                    lastLockTime +
267:                    uint32(_duration);
268:            }
269:        }
270:
271:        emit LockDuration(msg.sender, _duration);
272:    }

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

Tools Used

VS Code

Recommended Mitigation Steps

Update the new unlock time with uint32(block.timestamp) + uint32(_duration) rather than lastLockTime + uint32(_duration).

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

An oracle can approve an USD price even if it has disapproved the price already.

Proof of Concept

When an oracle approves an USD price, function approveUSDPrice only check if the oracle has approved the price already, and reverts the tx if it has. But the function does not check if the oracle has disapproved the price already. As a result, the oracle can first disapprove the price, and then approve the price again. This may mess up the pricing of tokens.

177:    function approveUSDPrice(
178:        uint256 _price
179:    )
180:        external
181:        onlyOneOfRoles(
182:            [
183:                Role.PriceFeed_1,
184:                Role.PriceFeed_2,
185:                Role.PriceFeed_3,
186:                Role.PriceFeed_4,
187:                Role.PriceFeed_5
188:            ]
189:        )
190:    {
191:        if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
192:        if (usdUpdateProposal.proposer == msg.sender)
193:            revert ProposerCannotApproveError();
194:@>      if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
195:            revert ProposalAlreadyApprovedError();
196:        if (usdUpdateProposal.proposedPrice != _price)
197:            revert ProposalPriceNotMatchedError();
198:
199:        usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
200:        usdUpdateProposal.approvalsCount++;

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

Tools Used

VS Code

Recommended Mitigation Steps

In function approveUSDPrice, check if the oracle has disapproved the USD price, and revert the tx if it has.

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

Assessed type

Other