Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Withdrawal from vMaia vault only on first Tuesday of the month is not strictly enforced #396

Closed
code423n4 opened this issue Jun 30, 2023 · 8 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-469 grade-c satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

The withdrawal of Maia ERC-20 tokens from the vMaia ERC-4626 vault should only be possible on the first Tuesday of the month.
In fact, the withdrawal is possible on the first Tuesday of attempted withdrawal of the month, due to the insufficient check in L109/110 of vMaia.beforeWithdraw(...) which only requires the current day to be a Tuesday.
Example: In case no Maia DAO member withdraws his Maia tokens on the first Tuesday of the month, withdrawal on the second Tuesday of the month and so forth is still possible.

It is stated at multiple instances throughout the ecosystem that the withdrawal is only possible on the first Tuesday of the month:

  • In code: L23/24 and L100 of vMaia.sol
  • In readme: L200 of README.md
  • In documentation: vMaia tokenomics (mistake: Monday instead of Tuesday is written there as of 2023-06-30)

Therefore a base assumption for DAO members about the Maia ecosystem is broken. Potential withdrawals outside the specified time frame might cause problems for the front-end as well as lead to an unfair advantage for users who know about this loophole in contrast to those who don't.

Proof of Concept

The following test case demonstrates that a withdrawal is also possible on the second Tuesday of the month in case there was no withdrawal on the first. Just add this test case to test/maia/vMaiaTest.t.sol:vMaiaTest and run it with forge test -vv --match-test testWithdrawMaiaOnSecondTuesday:

function testWithdrawMaiaOnSecondTuesday() public {
    testDepositMaia();

    uint256 amount = 100 ether;

    hevm.warp(getFirstDayOfNextMonthUnix() + 7 days); // add 7 days -> second Tuesday

    vmaia.withdraw(amount, address(this), address(this));

    assertEq(maia.balanceOf(address(vmaia)), 0);
    assertEq(vmaia.balanceOf(address(this)), 0);
}

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

The issue can be resolved by strictly enforcing the first Tuesday of the month:

diff --git a/src/maia/libraries/DateTimeLib.sol b/src/maia/libraries/DateTimeLib.sol
index 29e6910..751f754 100644
--- a/src/maia/libraries/DateTimeLib.sol
+++ b/src/maia/libraries/DateTimeLib.sol
@@ -52,11 +52,12 @@ library DateTimeLib {

     /// @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) {
+    function isFirstTuesdayOfMonth(uint256 timestamp) internal pure returns (bool result, uint256 startOfDay) {
         unchecked {
             uint256 day = timestamp / 86400;
             startOfDay = day * 86400;
-            result = ((day + 3) % 7) + 1 == 2;
+            // check if day is a Tuesday and previous Tuesday was previous month
+            result = ((day + 3) % 7) + 1 == 2 && getMonth(timestamp) == getMonth(timestamp - 7 * 86400) + 1;
         }
     }
 }
diff --git a/src/maia/vMaia.sol b/src/maia/vMaia.sol
index 3aa70cf..592a9ed 100644
--- a/src/maia/vMaia.sol
+++ b/src/maia/vMaia.sol
@@ -106,7 +106,7 @@ contract vMaia is ERC4626PartnerManager {
         uint256 _currentMonth = DateTimeLib.getMonth(block.timestamp);
         if (_currentMonth == currentMonth) revert UnstakePeriodNotLive();

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

         currentMonth = _currentMonth;

Assessed type

Invalid Validation

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jun 30, 2023
code423n4 added a commit that referenced this issue Jun 30, 2023
@c4-judge c4-judge removed the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Jul 11, 2023
@c4-judge
Copy link
Contributor

trust1995 changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jul 11, 2023
@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Jul 11, 2023
@c4-judge
Copy link
Contributor

trust1995 marked the issue as grade-c

@c4-judge c4-judge reopened this Jul 11, 2023
@c4-judge c4-judge reopened this Jul 11, 2023
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jul 11, 2023
@c4-judge
Copy link
Contributor

This previously downgraded issue has been upgraded by trust1995

1 similar comment
@c4-judge
Copy link
Contributor

This previously downgraded issue has been upgraded by trust1995

@c4-judge
Copy link
Contributor

trust1995 marked the issue as primary issue

@c4-judge c4-judge added primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards and removed unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Jul 11, 2023
@c4-judge
Copy link
Contributor

trust1995 marked the issue as satisfactory

@trust1995
Copy link

Ignore grade-c, finding is valid Med.

@c4-judge c4-judge added duplicate-469 and removed primary issue Highest quality submission among a set of duplicates labels Jul 25, 2023
@c4-judge
Copy link
Contributor

trust1995 marked issue #469 as primary and marked this issue as a duplicate of 469

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-469 grade-c satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants