WithdrawTime is incorrectly determined during withdrawal request being submitted

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L176

Vulnerability details

Vulnerability Details

• The current version of AfEth.requestWithdraw() determines withdrawTime based on the whole amount of AfEth being requested. Since the lockedBalances are in vlCVX, _amount that has to be passed into withdrawTime() should be calculated like it's determined for an argument being passed into the AbstractStrategy(vEthAddress).requestWithdraw().

Impact

• Currently, an incorrectly determined amount will be stored in withdrawIdInfo[_withdrawId].withdrawTime and luckily, since AbstractStrategy(vEthAddress).requestWithdraw() has it's own logic for withdrawEpoch calculation and withdrawIdToWithdrawRequestInfo[_withdrawId] doesn't rely on precomputed withdrawIdInfo[_withdrawId].withdrawTime, withdrawals itself are not under miscalculation.

Recommended Mitigation Steps

• Short term:

  • 
    function requestWithdraw(uint256 _amount) external virtual { 
    --  uint256 withdrawTimeBefore = withdrawTime(_amount);
    ++  uint256 withdrawTimeBefore = withdrawTime(votiumWithdrawAmount);
    ....
    }

• Long term: N/A

Assessed type

Context

[elmutt]

https://github.com/asymmetryfinance/afeth/pull/160

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed

[c4-judge]

0xleastwood changed the severity to 3 (High Risk)

[romeroadrian]

@0xleastwood I think this should be more on the low side, withdrawTime is merely informative here. Withdrawals in Votium are determined by the epoch which is stored in the epoch field of the withdraw request (see https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L162)

[0xleastwood]

@0xleastwood I think this should be more on the low side, withdrawTime is merely informative here. Withdrawals in Votium are determined by the epoch which is stored in the epoch field of the withdraw request (see https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L162)

You're most definitely right! I have no idea why I overlooked this so much. My apologies for this. Downgrading this all to QA.

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[rvierdiiev]

@0xleastwood i do not think that impact is informative

from my report impact is more time for user to get their funds back as they can't withdraw

AfEth.requestWithdraw function calculates withdraw time incorrectly, which cause user to wait more time to receive funds.

[0xleastwood]

withdrawIdInfo[latestWithdrawId].withdrawTime is not being enforced anyway. Maybe this is an issue on it's own, but this is not what is being highlighted in this issue. However, please correct me if I have made the wrong assumption.

[rvierdiiev]

Indeed, you are correct.

[Rassska]

I'm again confused on this:

• How SCs relying on this know, what mapping to query in order to get the correct withdrawTime()? The only starting point here is AfEth manager, hence any deposits/withdrawals have to flow through it, because of it, SCs will query withdrawIdInfo as they should. if not why Asymmetry decides to store that info on AfEth manager's side? This issue is completely messing up any integrations with AfEth making it impossible to design proper architecture above AfEth. I agree, it's not a high severity issue(no thefts/locks) and wasn't submitted as, but it has to be a medium, since it would require complete redeploy(in this case an implementation has to be upgraded).

[0xleastwood]

I'm not exactly aware of what an integration would look like for the AfEth's manager side so I might defer this to @elmutt to better understand this.

[elmutt]

I'm again confused on this:

• How SCs relying on this know, what mapping to query in order to get the correct withdrawTime()? The only starting point here is AfEth manager, hence any deposits/withdrawals have to flow through it, because of it, SCs will query withdrawIdInfo as they should. if not why Asymmetry decides to store that info on AfEth manager's side? This issue is completely messing up any integrations with AfEth making it impossible to design proper architecture above AfEth. I agree, it's not a high severity issue(no thefts/locks) and wasn't submitted as, but it has to be a medium, since it would require complete redeploy(in this case an implementation has to be upgraded).

What is the question exactly?

withdrawTime() is to be used by frontend and requestWithdraw() to know: "if I call requestWithdraw(amount) now, how long will it be until that amount if fully unlocked and able to be withdrawn"

I agree that it was passing the wrong amount and this is a bug. Also I think this PR fixes it: https://github.com/asymmetryfinance/afeth/pull/160

WIthdraw time is enforced by the convex locking mechanism and we are just predicting when that will be. Does that make sense?

[Rassska]

I'm again confused on this:

• How SCs relying on this know, what mapping to query in order to get the correct withdrawTime()? The only starting point here is AfEth manager, hence any deposits/withdrawals have to flow through it, because of it, SCs will query withdrawIdInfo as they should. if not why Asymmetry decides to store that info on AfEth manager's side? This issue is completely messing up any integrations with AfEth making it impossible to design proper architecture above AfEth. I agree, it's not a high severity issue(no thefts/locks) and wasn't submitted as, but it has to be a medium, since it would require complete redeploy(in this case an implementation has to be upgraded).

What is the question exactly?

withdrawTime() is to be used by frontend and requestWithdraw() to know: "if I call requestWithdraw(amount) now, how long will it be until that amount if fully unlocked and able to be withdrawn"

I agree that it was passing the wrong amount and this is a bug.

WIthdraw time is enforced by the convex locking mechanism and we are just predicting when that will be. Does that make sense?

• So, it turns out, that the front-end will always give an incorrect info about the time, when certain amount of afETH should be finalized. And it also means, any future integrations with afETH also will be corrupted because of it. Thanks, @elmutt, for the context provided!

[rvierdiiev]

So, it turns out, that the front-end will always give an incorrect info about the time, when certain amount of afETH should be finalized. And it also means, any future integrations with afETH also will be corrupted because of it. Thanks, @elmutt, for the context provided!

This is true, i have explained that in #10

[elmutt]

I'm again confused on this:

• How SCs relying on this know, what mapping to query in order to get the correct withdrawTime()? The only starting point here is AfEth manager, hence any deposits/withdrawals have to flow through it, because of it, SCs will query withdrawIdInfo as they should. if not why Asymmetry decides to store that info on AfEth manager's side? This issue is completely messing up any integrations with AfEth making it impossible to design proper architecture above AfEth. I agree, it's not a high severity issue(no thefts/locks) and wasn't submitted as, but it has to be a medium, since it would require complete redeploy(in this case an implementation has to be upgraded).

What is the question exactly? withdrawTime() is to be used by frontend and requestWithdraw() to know: "if I call requestWithdraw(amount) now, how long will it be until that amount if fully unlocked and able to be withdrawn" I agree that it was passing the wrong amount and this is a bug. WIthdraw time is enforced by the convex locking mechanism and we are just predicting when that will be. Does that make sense?

• So, it turns out, that the front-end will always give an incorrect info about the time, when certain amount of afETH should be finalized. And it also means, any future integrations with afETH also will be corrupted because of it. Thanks, @elmutt, for the context provided!

Why will it be incorrect?

Once requestWithdraw() has been called we can know with certainty when we can call withdraw() by looking at the WithdrawRequest event emitted by requestWithdraw().

The only time it is unknown is before requestWithdraw() has been called, we explain this in the readme under "A note about unlock times":

[A note about varying unlock times](https://github.com/asymmetryfinance/afeth#a-note-about-varying-unlock-times)
When a user calls requestWithdraw() the contract looks at who has requested to withdraw before them, calculates the date at which enough vlcvx can be unlocked to close their position along with everyone in front of them, and marks that amount of convex to be unlocked asap.

Because of this, the withdraw time will be contantly changing for users that havent called requestWithdraw(). This could cause users to "race" to enter the unlock queue under certain unqiue market conditions.

While this isnt ideal, we do not believe it to be exploitable in a harmful way because the maximum unlock time is 16 weeks regardless of state of the unlock queue.

What am I missing?

[elmutt]

ohhh

I'm again confused on this:

• How SCs relying on this know, what mapping to query in order to get the correct withdrawTime()? The only starting point here is AfEth manager, hence any deposits/withdrawals have to flow through it, because of it, SCs will query withdrawIdInfo as they should. if not why Asymmetry decides to store that info on AfEth manager's side? This issue is completely messing up any integrations with AfEth making it impossible to design proper architecture above AfEth. I agree, it's not a high severity issue(no thefts/locks) and wasn't submitted as, but it has to be a medium, since it would require complete redeploy(in this case an implementation has to be upgraded).

What is the question exactly? withdrawTime() is to be used by frontend and requestWithdraw() to know: "if I call requestWithdraw(amount) now, how long will it be until that amount if fully unlocked and able to be withdrawn" I agree that it was passing the wrong amount and this is a bug. WIthdraw time is enforced by the convex locking mechanism and we are just predicting when that will be. Does that make sense?

• So, it turns out, that the front-end will always give an incorrect info about the time, when certain amount of afETH should be finalized. And it also means, any future integrations with afETH also will be corrupted because of it. Thanks, @elmutt, for the context provided!

ohhh are you saying it should be a medium or high issue instead of QA? I agree, its completely broken and break requestWithdraw() until our PR is merged.

[Rassska]

ohhh

I'm again confused on this:

• How SCs relying on this know, what mapping to query in order to get the correct withdrawTime()? The only starting point here is AfEth manager, hence any deposits/withdrawals have to flow through it, because of it, SCs will query withdrawIdInfo as they should. if not why Asymmetry decides to store that info on AfEth manager's side? This issue is completely messing up any integrations with AfEth making it impossible to design proper architecture above AfEth. I agree, it's not a high severity issue(no thefts/locks) and wasn't submitted as, but it has to be a medium, since it would require complete redeploy(in this case an implementation has to be upgraded).

What is the question exactly? withdrawTime() is to be used by frontend and requestWithdraw() to know: "if I call requestWithdraw(amount) now, how long will it be until that amount if fully unlocked and able to be withdrawn" I agree that it was passing the wrong amount and this is a bug. WIthdraw time is enforced by the convex locking mechanism and we are just predicting when that will be. Does that make sense?

• So, it turns out, that the front-end will always give an incorrect info about the time, when certain amount of afETH should be finalized. And it also means, any future integrations with afETH also will be corrupted because of it. Thanks, @elmutt, for the context provided!

ohhh are you saying it should be a medium or high issue instead of QA? I agree, its completely broken and break requestWithdraw() until our PR is merged.

• yeah, the question was about the severity behind the impact. Thanks again for such context!

[0xleastwood]

How does it break requestWithdraw()? We just emit the wrong event which means we might attempt to finalise the withdrawal at a later date. This is not enforced within the contract and these types of issues are typically graded as low severity.

[0xleastwood]

The protocol does not leak value nor does this issue affect liveness.

[rvierdiiev]

How does it break requestWithdraw()? We just emit the wrong event which means we might attempt to finalise the withdrawal at a later date. This is not enforced within the contract and these types of issues are typically graded as low severity.

pls, check #10 it breaks integration of other protocols with AfEth but yes, requestWithdraw function works fine, the problem is with withdrawTime function

[0xleastwood]

Yes, I understand that but there are some huge assumptions being made here. This is not affecting the protocol in its current state.

[Rassska]

Yes, I understand that but there are some huge assumptions being made here. This is not affecting the protocol in its current state.

• But how about the mapping, which is clearly filled with invalid withdrawTime attribute objects that is supposed to be used for any kind of integrations?

• Anyways, I respect your decision and will not argue further on it. Thanks!

[romeroadrian]

This is a nice catch, but withdrawTime isn't really used at all, and in fact it could be beneficial to completely remove it since Convex epochs are already tracked and those can be queried elsewhere.

Front-end or off-chain impact is usually considered low/QA (see https://github.com/code-423n4/org/issues/79).

[elmutt]

How does it break requestWithdraw()? We just emit the wrong event which means we might attempt to finalise the withdrawal at a later date. This is not enforced within the contract and these types of issues are typically graded as low severity.

ok fair enough, it is just a frontend issue then.

[Rassska]

This is a nice catch, but withdrawTime isn't really used at all, and in fact it could be beneficial to completely remove it since Convex epochs are already tracked and those can be queried elsewhere.

Front-end or off-chain impact is usually considered low/QA (see code-423n4/org#79).

• Really d? At that point, we simply can remove the whole AfEth manager and make deposits/withdrawals directly into each strategies? Why would we have to spend the gas for that additional level of abstraction? I don't think that it's totally UI/front-end issue, it corrupts the current state of the withdrawals being requested by providing false info about the finalization time.

• Front-end hugely relies on that state and thus gives an invalid assumptions. What should front-end do? Get the time directly from VotiumStrategy? If so, what the point of AfEth manager? What if new strategies have been added in the future? Will your front-end query all strategies separately or will it use a higher level of abstraction which is called AfEth manager which is exactly build for that?

[0xleastwood]

This is a nice catch, but withdrawTime isn't really used at all, and in fact it could be beneficial to completely remove it since Convex epochs are already tracked and those can be queried elsewhere.

Front-end or off-chain impact is usually considered low/QA (see code-423n4/org#79).

Yep this is typically how we grade these issues.

[0xleastwood]

Yes the frontend provides some incorrect information about when a withdrawal is finalised, but if you access the manager contract directly or sign the tx to finalise the withdrawal, it will still execute

[0xleastwood]

Leaving this issue as is.

[Rassska]

Yes the frontend provides some incorrect information about when a withdrawal is finalised, but if you access the manager contract directly or sign the tx to finalise the withdrawal, it will still execute

If you access the manager directly, it will return the wrongly computed withdrawTime, because it stores the wrongly computed withdrawTimeBefore here: https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L207

[0xleastwood]

Okay, but that does not prevent you from withdrawing does it?

[Rassska]

Okay, but that does not prevent you from withdrawing does it?

Well, if it stops the withdrawal from being finalized, without a doubt the impact should be high. It doesn't stop, but the manager, which is supposed to hold the correct state over all requested withdrawals, fails to deliver it, because of the wrong calculation being made. And I'm pretty sure, that Asymmetry 100% would fix this issue by upgrading/redeploying implementation, if it wasn't catched here. @elmutt, @toshiSat, pls, confirm it.

It's pretty sad to see, when the obvious calculation issues are being discarded and on the other hand the issues that do not or hypothetically exist in a system being accepted(not only my pov, but the sponsor's/other wardens) as valid. :(

[0xleastwood]

I'm not saying it's not an issue, but you are elevating the impact of the issue. The user is not unable to call withdraw() when the manager contract is able to fulfil the request even if the user may be told "off-chain" the wrong point in time where they should be expected to be able to withdraw their funds.

[0xleastwood]

If a sponsor chooses to fix an issue, it does not qualify it for medium severity. The impact of this issue is minimal in its current form.

[0xleastwood]

Just to clarify, other issues which I have accepted as medium are not hypothetical, they are valid ways to disrupt the system or do something abnormal which imo is not in-line with the intended use of the protocol. They are things that can happen whereas this issue is something that might happen but we need to add other assumptions and dependencies for it to cause any issues.