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

unsafe cast can lead to theft #224

Closed
code423n4 opened this issue Feb 9, 2022 · 1 comment
Closed

unsafe cast can lead to theft #224

code423n4 opened this issue Feb 9, 2022 · 1 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 This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L261
https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L235

Vulnerability details

in the first link I provided, a hacker can call withdraw with amount = 2**196, the amount of shares that will be burned is uint192(2**192) == 0.
if the system has enough money, the hacker can steal 2**192 tokens and pay nothing.

for the second link, a user that provides more than 2**192 tokens, the amount of shares they will be truncated and they will get less than they should.

Recommended Mitigation Steps

use safe cast of openzeppelin.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Feb 9, 2022
code423n4 added a commit that referenced this issue Feb 9, 2022
@r2moon r2moon added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 16, 2022
@GalloDaSballo
Copy link
Collaborator

GalloDaSballo commented Apr 19, 2022

While the warden intuition is correct, the lack of POC makes the finding fall under scrutiny.
The casting on the ConvexStakingWrapper is to uint192 however the masterchef contract which will be notified of the withdrawal uses uint128 meaning that the shown POC will revert and won't allow for any value extraction.

If a common denominator between the two where to be found such that _amount was zero in both contracts, then value extraction could happen.

However this POC is not developed enough to sustain this reasoning.

Because of that I'm going to mark this as duplicate of #194 as the casting problem is more developed there

@GalloDaSballo GalloDaSballo added duplicate This issue or pull request already exists 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Apr 19, 2022
@JeeberC4 JeeberC4 removed the 3 (High Risk) Assets can be stolen/lost/compromised directly label Apr 27, 2022
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 This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

4 participants