Withdrawal from vMaia position is possible on one of any Tuesday of the month in some cases (e.g. second or third Tuesday) which goes against how the protocol should work

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/vMaia.sol#L109-L110

Vulnerability details

Impact

beforeWithdraw function performs the necessary verifications before a user can withdraw from their vMaia position. Basically, it checks if we're inside the unstaked period, if so then the user is able to withdraw.

	/// @dev Check if unstake period has not ended yet, continue if it is the case.
	if (unstakePeriodEnd >= block.timestamp) return;

	uint256 _currentMonth = DateTimeLib.getMonth(block.timestamp);
	if (_currentMonth == currentMonth) revert UnstakePeriodNotLive();

Then it checks if it is Tuesday

	(bool isTuesday, uint256 _unstakePeriodStart) = DateTimeLib.isTuesday(block.timestamp);
	if (!isTuesday) revert UnstakePeriodNotLive();

https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/vMaia.sol#L109-L110

However, it doesn't check if it is the first Tuesday. According to Maia, unstaking is done on 1st Tuesday each month. Here is the link of the tweet:
https://twitter.com/MaiaDAOEco/status/1664383658935894016

Another source to confirm this is a comment in the code:

    /// @dev Error thrown when trying to withdraw and it is not the first Tuesday of the month.
    error UnstakePeriodNotLive();

https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/vMaia.sol#L120-L121

Because of this bug, the users can withdraw on any Tuesday of the month (but only one) which goes against how the protocol should work potentially causing harm economically.

In other words, if there was no withdrawal on the first Tuesday of the month, then the users will be able to withdraw on the second Tuesday. If no withdrawal occurs on the second, the third then and so on.

Proof of Concept

If you have a look at the function DateTimeLib.isTuesday that's used by beforeWithdraw, you can see that it takes a date (timestamp), does a calculation to return a number. if the number is 2 , it means it is Tuesday.

	/// @dev Returns the weekday from the unix timestamp.
	/// Monday: 1, Tuesday: 2, ....., Sunday: 7.
	function isTuesday(uint256 timestamp) internal pure returns (bool result, uint256 startOfDay) {
		unchecked {
			uint256 day = timestamp / 86400;
			startOfDay = day * 86400;
			result = ((day + 3) % 7) + 1 == 2;
		}
	}

https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/libraries/DateTimeLib.sol#L55-L60

This is actually inspired by Solady

	/// @dev Returns the weekday from the unix timestamp.
	/// Monday: 1, Tuesday: 2, ....., Sunday: 7.
	function weekday(uint256 timestamp) internal pure returns (uint256 result) {
		unchecked {
			result = ((timestamp / 86400 + 3) % 7) + 1;
		}
	}

https://github.com/Vectorized/solady/blob/main/src/utils/DateTimeLib.sol#L192-L198

To prove the issue above we will go through two scenarios. One that works as intended and the other where the issue occurs.

Working as intended

In the following flow, the withdrawal will revert on the second Tuesday

Assume

1. unstakePeriodEnd => 0
2. currentMonth => 0
3. Today => 03/07 Monday

Now go through the following flow:

• Withdraw [ Today => 03/07 Monday ]
  • beforeWithdraw
    • revert if currentMonth == getMonth(block.timestamp)
      • 0 == 7 => false => continue
    • revert if not isTuesday(block.timestamp)
      • Tuesday == Monday => false => revert
• Withdraw [ Today => 04/07 First Tuesday of the month ]
  • beforeWithdraw
    • revert if currentMonth == getMonth(block.timestamp)
      • 0 == 7 => false => continue
    • revert if not isTuesday(block.timestamp)
      • Tuesday == Tuesday => true => continue
    • Update State
      • currentMonth = 7
      • unstakePeriodEnd = last second of Tuesday
  • Back to withdraw
• Withdraw [ Today => 05/07 Wednesday ]
  • beforeWithdraw
    • revert if currentMonth == getMonth(block.timestamp)
      • 7 == 7 => true => revert
• .
• .
• .
• Withdraw [ Today => 11/07 Second Tuesday of the month ]
  • beforeWithdraw
    • revert if currentMonth == getMonth(block.timestamp)
      • 7 == 7 => true => revert

Not working as intended

In the following flow, the withdrawal will not revert on the second Tuesday

Assume

1. unstakePeriodEnd => 0
2. currentMonth => 0
3. Today => 03/07 Monday

Now go through the following flow:

• Withdraw [ Today => 03/07 Monday ]
  • beforeWithdraw
    • revert if currentMonth == getMonth(block.timestamp)
      • 0 == 7 => false => continue
    • revert if not isTuesday(block.timestamp)
      • Tuesday == Monday => false => revert
• No withdrawal occured [ Today => 04/07 First Tuesday of the month ]
  • Note that state vars have not been updated
• Withdraw [ Today => 05/07 Wednesday ]
  • beforeWithdraw
    • revert if currentMonth == getMonth(block.timestamp)
      • 0 == 7 => false => continue
    • revert if not isTuesday(block.timestamp)
      • Tuesday == Wednesday => false => revert
• .
• .
• .
• Withdraw [ Today => 11/07 Second Tuesday of the month ]
  • beforeWithdraw
    • revert if currentMonth == getMonth(block.timestamp)
      • 0 == 7 => false => continue
    • revert if not isTuesday(block.timestamp)
      • Tuesday == Tuesday => true => continue
    • Update State
      • currentMonth = 7
      • unstakePeriodEnd = last second of Tuesday
  • Back to withdraw

From the flow above, we showed a case where the issue occurs
Note: if the second Tuesday passed with no withdrawals, then the users will be able to withdraw on the third Tuesday and so on.

Tools Used

Manual analysis

Recommended Mitigation Steps

Check if the day of Tuesday is from the first seven days of the month. This way, you guarantee that it is always the first Tuesday of the month. You can do this by extracting the dd from the date dd/mm/yyyy, then check if dd is lower than 8

Assessed type

Invalid Validation

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-c

[c4-judge]

This previously downgraded issue has been upgraded by trust1995

[c4-judge]

This previously downgraded issue has been upgraded by trust1995

[c4-judge]

trust1995 marked the issue as duplicate of #396

[c4-judge]

trust1995 marked the issue as satisfactory