Summary
A user copy data that is outside of the copy data array using the clipboard and can even corrupt data that is next to the data to paste
Relevant GitHub Links:
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/main/protocol/contracts/libraries/LibBytes.sol#L215
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/main/protocol/contracts/libraries/LibBytes.sol#L223
Vulnerability Details
When an advanced call is executed, it may use the clipboard functionality. This functionality allow a user to copy bytes of returned data from previous calls to the calldata for the next calls.
To determine how much data to copy, from where and to which calldata, it must be determined by 3 variables called copyReturnIndex, copyByteIndex and pasteByteIndex.
There are 2 variables in question that are pretty important to constrain properly, those are copyByteIndex and pasteByteIndex.
The first one tells the byte location from where to copy 32 bytes and the second one the byte index of where to paste these copied 32 bytes.
The requirements for these 2 variables are the following ones:
uint80 internal constant SLOT_SIZE = 32;
function verifyCopyByteIndex(uint256 copyByteIndex, bytes memory copyFromData) internal pure {
require(C.SLOT_SIZE <= copyByteIndex, "LibBytes: copyByteIndex too small");
require(copyByteIndex <= copyFromData.length, "LibBytes: copyByteIndex too large");
}
function verifyPasteByteIndex(uint256 pasteByteIndex, bytes memory pasteToData) internal pure {
require(C.SLOT_SIZE <= pasteByteIndex, "LibBytes: pasteByteIndex too small");
require(pasteByteIndex <= pasteToData.length, "LibBytes: pasteByteIndex too large");
}
As we can see the only constraints are that the start of the bytes to copy is after the first 32 bytes and the last to copy is the length of the whole array of bytes.
The way it copies the data is with the following function:
function paste32Bytes(
bytes memory copyFromData,
bytes memory pasteToData,
uint256 copyIndex,
uint256 pasteIndex
) internal pure {
assembly {
mstore(add(pasteToData, pasteIndex), mload(add(copyFromData, copyIndex)))
}
}
This essentially copies 32 bytes accordingly to the indexes passed to the function.
Taking into account these 3 functions it is possible to copy data that should not be copied or corrupt data that should remain untouched. That is because the starting byte index to copy or to paste + the 32 bytes can get out of this byte array.
Example:
Current memory arranging
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
0000000000000000000000000000000000000000000000000000000000000002 ───┐
0000000000000000000000000000000000000000000000000000000000000003 │ Data to copy from
0000000000000000000000000000000000000000000000000000000000000004 │
0000000000000000000000000000000000000000000000000000000000000005 ───┘
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
0000000000000000000000000000000000000000000000000000000000000008 ───┐
0000000000000000000000000000000000000000000000000000000000000009 │
000000000000000000000000000000000000000000000000000000000000000a │ Data to paste to
000000000000000000000000000000000000000000000000000000000000000b │
000000000000000000000000000000000000000000000000000000000000000c │
000000000000000000000000000000000000000000000000000000000000000d ───┘
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
Imagine that the copyByteIndex is greater than 96 and less than 128, let's say 100, it would copy 00000000000000000000000000000000000000000000000000000005ffffffff because of the adjacent data that is next to it. In this case this will be bad because it is being copied data that was not expected to be copied and it depends on what is next to the data so it can lead to unexpected results for the user.
There is a also an other really bad case, where data is corrupted.
Example:
Current memory arranging
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
0000000000000000000000000000000000000000000000000000000000000002 ───┐
0000000000000000000000000000000000000000000000000000000000000003 │ Data to copy from
0000000000000000000000000000000000000000000000000000000000000004 │
0000000000000000000000000000000000000000000000000000000000000005 ───┘
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
0000000000000000000000000000000000000000000000000000000000000008 ───┐
0000000000000000000000000000000000000000000000000000000000000009 │
000000000000000000000000000000000000000000000000000000000000000a │ Data to paste to
000000000000000000000000000000000000000000000000000000000000000b │
000000000000000000000000000000000000000000000000000000000000000c │
000000000000000000000000000000000000000000000000000000000000000d ───┘
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
Imagine that the copyByteIndex in this situation is 32 and the pasteByteIndex is greater than 160 and less than 192, let's say 170. It would copy this word full of 0xa to the data to paste and half of data that is next to it that will be corrupted.
Final memory arranging
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
0000000000000000000000000000000000000000000000000000000000000002 ───┐
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa │ Data to copy from
0000000000000000000000000000000000000000000000000000000000000004 │
0000000000000000000000000000000000000000000000000000000000000005 ───┘
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
0000000000000000000000000000000000000000000000000000000000000008 ───┐
0000000000000000000000000000000000000000000000000000000000000009 │
000000000000000000000000000000000000000000000000000000000000000a │ Data to paste to
000000000000000000000000000000000000000000000000000000000000000b │
000000000000000000000000000000000000000000000000000000000000000c │
00000000000000000000aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ───┘
aaaaaaaaaaaaaaaaaaaaffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
Impact
Medium, a user can get unexpected from the advanced calls.
Tools Used
Manual review
Recommendations
Set the maximum index to the length of the data array - 32:
function verifyCopyByteIndex(uint256 copyByteIndex, bytes memory copyFromData) internal pure {
require(C.SLOT_SIZE <= copyByteIndex, "LibBytes: copyByteIndex too small");
- require(copyByteIndex <= copyFromData.length, "LibBytes: copyByteIndex too large");
+ require(copyByteIndex <= copyFromData.length - 32, "LibBytes: copyByteIndex too large");
}
function verifyPasteByteIndex(uint256 pasteByteIndex, bytes memory pasteToData) internal pure {
require(C.SLOT_SIZE <= pasteByteIndex, "LibBytes: pasteByteIndex too small");
- require(pasteByteIndex <= pasteToData.length, "LibBytes: pasteByteIndex too large");
+ require(pasteByteIndex <= pasteToData.length - 32, "LibBytes: pasteByteIndex too large");
}
This way the copy index and the paste index would always be inside the data array and could not exceed from there.
