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

Wrong time calculating in proposeOffboard() #583

Closed
c4-bot-6 opened this issue Dec 26, 2023 · 7 comments
Closed

Wrong time calculating in proposeOffboard() #583

c4-bot-6 opened this issue Dec 26, 2023 · 7 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-816 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-6
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L89-L97
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L116-L123

Vulnerability details

Impact

In LendingTermOffboarding contract, users could call proposeOffboard() function, but they could do it, if current block.number is greater than time from mapping lastPollBlock[term] plus constant value , which stored in POLL_DURATION_BLOCKS.

    function proposeOffboard(address term) external whenNotPaused {
         ...
        require(
            block.number > lastPollBlock[term] + POLL_DURATION_BLOCKS,
            "LendingTermOffboarding: poll active"
        );

There problem is, that project will be deployed on different blockchains, such as ethereum and arbitrum. In these chains there are different time of mining new blocks. Values vary greatly.
In arbitrum minting blocks is more faster, that in ethereum, so users could call proposeOffboard() after lastPollBlock[term] + ~7 days ( 13s/block), but in arbitrum: lastPollBlock[term] + ~1 day ( 2s/block)

The same problem there is in function supportOffboard().

    function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
        require(
            block.number <= snapshotBlock + POLL_DURATION_BLOCKS,
            "LendingTermOffboarding: poll expired"
        );

Users will have shorter period for supporting on arbitrum network, than in ethereum.

Proof of Concept

You can see minting block times on https://etherscan.io/ for ethereum and on https://arbiscan.io/ for arbutrum.

Tools Used

Manual review

Recommended Mitigation Steps

Make the constant a variable. Set value in constructor, because this value will be different for each blockchain

Assessed type

Invalid Validation

@c4-bot-6 c4-bot-6 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 Dec 26, 2023
c4-bot-1 added a commit that referenced this issue Dec 26, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Jan 4, 2024
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as duplicate of #816

@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 and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jan 24, 2024
@c4-judge
Copy link
Contributor

Trumpero changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

Trumpero marked the issue as grade-b

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards and removed grade-b labels Jan 31, 2024
@c4-judge
Copy link
Contributor

Trumpero marked the issue as grade-c

@SovaSlava
Copy link

Hello @Trumpero !
Why this issue has grade-c, while other issues with such duplicate have grade-a/grade-b?

@Trumpero
Copy link

Trumpero commented Feb 3, 2024

If issues are downgraded to be a QA, they will be included in the QA report of the warden. They will still be counted even when the warden doesn't submit a QA report. Each judge has a different evaluation system to assign QA points. In my system, it's 5 points for low severity and 1 point for refactor/NC. Since this low issue is your only issue, your total QA points is 5, resulting in a grade-c. Therefore, all of your QA-report issues will be marked as grade-c.
In my evaluation of this contest, you need to score more than 50% compared to the best QA report to achieve a grade-b. For grade-a, the threshold is 75%.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-816 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

5 participants