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

OZ L02; Spearbit 87; improved calldata checks #316

Closed
wants to merge 20 commits into from

Conversation

hensha256
Copy link
Contributor

No description provided.

@hensha256 hensha256 changed the title improved calldata checks OZ L01 improved calldata checks Aug 30, 2024
@hensha256 hensha256 changed the title OZ L01 improved calldata checks OZ L02 improved calldata checks Aug 30, 2024
@hensha256 hensha256 changed the title OZ L02 improved calldata checks OZ L02; Spearbit 87; improved calldata checks Aug 30, 2024
assembly ("memory-safe") {
res.length := length
res.offset := offset
let bytesOffset := and(_bytes.offset, OFFSET_OR_LENGTH_MASK)
Copy link

Choose a reason for hiding this comment

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

Masking done here but not in toBytes--intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have pushed an update for that 2fd239f

0age
0age previously approved these changes Sep 5, 2024
returns (uint256 length, uint256 offset)
{
function toBytes(bytes calldata _bytes, uint256 _arg) internal pure returns (bytes calldata res) {
uint256 length;
uint256 relativeOffset;
assembly ("memory-safe") {
// The offset of the `_arg`-th element is `32 * arg`, which stores the offset of the length pointer.
// shl(5, x) is equivalent to mul(32, x)
let lengthPtr := add(_bytes.offset, calldataload(add(_bytes.offset, shl(5, _arg))))
Copy link
Contributor

Choose a reason for hiding this comment

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

if you give a very high offset value for the bytes here, you could overflow this value; could apply the mask to the value, or check that it matches some fixed value (especially if it's a sole argument in which case it'd always be 0x20)

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 think youre commenting on an old version - i apply the mask now. Added in 2fd239f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh do you mean mask arg? i'm not too concerned about that as we will always pass in that in internally to the code

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah i think this comment is outdated now!

0age
0age previously approved these changes Sep 5, 2024

bytes memory invalidParams = new bytes(params.length - 1);
// dont copy the final byte
for (uint256 i = 0; i < params.length - 2; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

ayyyyy

}
if (_bytes.length < length + relativeOffset) revert SliceOutOfBounds();
}

/// @notice Decode the `_arg`-th element in `_bytes` as `bytes`
/// @param _bytes The input bytes string to extract a bytes string from
/// @param _bytes The input bytes string to extract a bytes array from
/// @param _arg The index of the argument to extract
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment for when this is expected to revert with the SliceOutOfBounds error?

@kmbarry1
Copy link

kmbarry1 commented Sep 5, 2024

I think actions and params could overlap even with the improved checks, e.g. if the head of actions just points at one of the elements of params. I'm not sure that's problematic per se (based on usage in the code, which is read-only AFAICT), but it is weird.

Copy link
Member

@snreynolds snreynolds left a comment

Choose a reason for hiding this comment

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

i have reviewed just the tests which look good to me!

@hensha256
Copy link
Contributor Author

closing for #346 due to @kmbarry1 's comment above

@hensha256 hensha256 closed this Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants