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

loss of gas for the user because of not using msg.value #130

Closed
Tracked by #88
code423n4 opened this issue Oct 23, 2022 · 3 comments
Closed
Tracked by #88

loss of gas for the user because of not using msg.value #130

code423n4 opened this issue Oct 23, 2022 · 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 disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) invalid This doesn't seem right responded The Holograph team has reviewed and responded 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-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/module/LayerZeroModule.sol#L241

Vulnerability details

Impact

  • L241 - An ether is sent using an input of the function, it should be validated before if msgValue >= msg.value and throw a corresponding exception.
    This could cause problems, since if the user sends 10,000 weis and puts the value 1,000 weis in the parameter, he would be losing 9,000 weis in the contract that he could not recover.
    This is necessary to do since we cannot trust the Operator contract to validate it, the implementation may change.

Recommended Mitigation Steps

Make the send() function either request that msg.value == msgValue or directly use msg.value or as a last possible solution, make it return the non-excess gas.

@code423n4 code423n4 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 Oct 23, 2022
code423n4 added a commit that referenced this issue Oct 23, 2022
@gzeoneth
Copy link
Member

Valid but low risk.

@gzeoneth gzeoneth added the disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) label Oct 31, 2022
@alexanderattar
Copy link

Low risk, but true. The suggestion above can be added.

@alexanderattar alexanderattar added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") responded The Holograph team has reviewed and responded labels Nov 8, 2022
@ACC01ADE
Copy link

ACC01ADE commented Nov 18, 2022

Upon further review, (LayerZeroModule.sol#L227)(https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/module/LayerZeroModule.sol#L227) is exclusively used in HolographOperator.sol#L640. The msgValue is guaranteed to be valid and same as msg.value. No changes will be implemented to address this issue since it's not one.

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 disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) invalid This doesn't seem right responded The Holograph team has reviewed and responded 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