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

Use of depreciated OZ Function isContract(), can be bypassed #96

Closed
c4-bot-10 opened this issue Mar 21, 2024 · 3 comments
Closed

Use of depreciated OZ Function isContract(), can be bypassed #96

c4-bot-10 opened this issue Mar 21, 2024 · 3 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 edited-by-warden insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-10
Copy link
Contributor

c4-bot-10 commented Mar 21, 2024

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/bridge/Bridge.sol#L493

Vulnerability details

Impact

Bypass Logic to Verify if Caller is a Contract (Deprecated by OZ)

Proof of Concept

The processMessage() calls the _invokeMessageCall() function to invoke the message call which in turn checks if the to value is a contract or not and if it is, then it assigns false value to success_ parameter (L495).

However, OZ's official docs state that isContract function can return false for an address where a contract lived but, but was destroyed and also, the function has been deprecated. In which case the following condition:

     if (
            _message.data.length >= 4 // msg can be empty
                && bytes4(_message.data) != IMessageInvocable.onMessageInvocation.selector
                && _message.to.isContract()
        ) {
            success_ = false;
        } else {
            --snip--
        }

will resolve as false and the funds will get transferred regardless.

Tools Used

Manual audit (VS Code)

Recommended Mitigation Steps

Make a function that verifies their EOA status with 2 calls that must be on 2 different blocks.

Assessed type

call/delegatecall

@c4-bot-10 c4-bot-10 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 Mar 21, 2024
c4-bot-10 added a commit that referenced this issue Mar 21, 2024
@c4-pre-sort
Copy link

minhquanym marked the issue as insufficient quality report

@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Mar 28, 2024
@minhquanym
Copy link
Member

Consider QA

@c4-judge
Copy link
Contributor

c4-judge commented Apr 8, 2024

0xean marked the issue as unsatisfactory:
Overinflated severity

@c4-judge c4-judge closed this as completed Apr 8, 2024
@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Apr 8, 2024
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 edited-by-warden insufficient quality report This report is not 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