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

nmirchev8_ctf -Fixed Return Data Size Allocation #29

Open
Chidubemkingsley opened this issue Aug 12, 2024 · 2 comments
Open

nmirchev8_ctf -Fixed Return Data Size Allocation #29

Chidubemkingsley opened this issue Aug 12, 2024 · 2 comments
Labels
bug Something isn't working duplicate This issue or pull request already exists partial

Comments

@Chidubemkingsley
Copy link

Severity: Medium

Vulnerability Details:
Even after fixing the dynamic size allocation, there is a bug where retData is still pre-allocated to a fixed size (2 * 32 bytes). This allocation should be done dynamically based on returndatasize() after the call is made. The return data (retData) is pre-allocated to maxReturnDataBytes (64 bytes).
This fixed allocation could lead to inefficient memory usage and potential issues if the actual return data is much smaller or larger.
Proof Of Code

uint16 maxReturnDataBytes = 2 * 32;
retData = new bytes(maxReturnDataBytes);

Impact:
Inefficiency: Allocating more memory than needed wastes gas and can potentially cause problems if not managed properly.
Potential Data Truncation: The current allocation doesn't handle return data that exceeds 64 bytes.

Tool Used
Manual

Recommendation
Allocate memory for retData dynamically after determining the size using returndatasize():

let returnDataSize := returndatasize()
retData := new bytes(returnDataSize)
mstore(retData, returnDataSize)
returndatacopy(add(retData, 0x20), 0x0, returnDataSize)

This approach ensures that the contract efficiently handles return data of any size without risk of truncation or inefficiency.

@NicolaMirchev
Copy link
Contributor

I am very skeptical about how to judge your submission.
You have found the problem part, but you didn't point out the exact impact and based on your description, you don't understand the purpose behind the contract functionality. Truncating the response is part of the main functionality, because we want to protect from gas bombs.
#34 best describes the issue, the root and the impact.

I've decided to split the reward between the two submissions. Although yours was the first, the description was lacking in detail and main impact.

@NicolaMirchev NicolaMirchev added bug Something isn't working duplicate This issue or pull request already exists partial labels Aug 17, 2024
@Chidubemkingsley
Copy link
Author

I am very skeptical about how to judge your submission. You have found the problem part, but you didn't point out the exact impact and based on your description, you don't understand the purpose behind the contract functionality. Truncating the response is part of the main functionality, because we want to protect from gas bombs. #34 best describes the issue, the root and the impact.

I've decided to split the reward between the two submissions. Although yours was the first, the description was lacking in detail and main impact.

Thank you for judging fairly. i appreciate your decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working duplicate This issue or pull request already exists partial
Projects
None yet
Development

No branches or pull requests

2 participants