Skip to content

Conversation

@mrice32
Copy link
Contributor

@mrice32 mrice32 commented Apr 15, 2022

This addresses two potential cases where tokens get locked:

  1. A caller incorrectly sets isWrappedMatic to false, but provides the wMatic token address.
  2. ERC20 tokens are erroneously transferred to the contract.

In both cases, this ensures that the tokens make it to the HubPool contract.

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
@nicholaspai
Copy link
Member

Can you explain the failure case this addresses?

@mrice32
Copy link
Contributor Author

mrice32 commented Apr 15, 2022

Can you explain the failure case this addresses?

@nicholaspai great point. Added a description that describes this. PTAL.

Comment on lines 94 to 97
if (isWrappedMatic) {
uint256 balance = address(this).balance;
maticToken.withdraw{ value: balance }(balance);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: does this need to be a separate state variable? could it not be:

Suggested change
if (isWrappedMatic) {
uint256 balance = address(this).balance;
maticToken.withdraw{ value: balance }(balance);
}
if (isWrappedMatic) maticToken.withdraw{ value: address(this).balance }(balance);

Copy link
Member

@nicholaspai nicholaspai Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable and saves an MSTORE right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the above works, since balance doesn't exist. Maybe you meant:

if (isWrappedMatic) maticToken.withdraw{ value: address(this).balance }(address(this).balance);

?

We could do that. I'm not sure it would be cheaper, but that might not matter since this is on polygon and the call is already pretty cheap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ye, sorry. it's hard to structure feedback in these comments with the proposed blocks. what you did is what I had intended.

mrice32 added 2 commits April 19, 2022 19:08
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
@mrice32 mrice32 requested a review from chrismaree April 19, 2022 23:30
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
@mrice32 mrice32 added the OZ Audit - March Resolves issue discovered in March 2022 OZ Audit label Apr 19, 2022
Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and clean, LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OZ Audit - March Resolves issue discovered in March 2022 OZ Audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants