Summary
The SablierFlow contract allows the sender of a stream to initiate a rate adjustment without requiring the approval of the recipient or admin. While the contract has been updated to include a mechanism for initiating and approving rate adjustments, there are concerns about the potential for malicious behaviour, such as adjusting the rate to an undesirable value without the knowledge or consent of the other party involved in the transaction. This could result in financial loss or exploitation of the recipient.
Vulnerability Details
ratePerSecond Adjustment Without Approval: The sender can initiate a rate adjustment using `adjustRatePerSecond` without any requirement for approval. The recipient has no control over the initiation of this adjustment.
Vulnerability can be found here:https://github.com/Cyfrin/2024-10-sablier/blob/8a2eac7a916080f2022527408b004578b21c51d0/src/SablierFlow.sol#L542-L561
@> function _adjustRatePerSecond(uint256 streamId, UD21x18 newRatePerSecond) internal {
if (newRatePerSecond.unwrap() == _streams[streamId].ratePerSecond.unwrap()) {
revert Errors.SablierFlow_RatePerSecondNotDifferent(streamId, newRatePerSecond);
}
uint256 ongoingDebtScaled = _ongoingDebtScaledOf(streamId);
if (ongoingDebtScaled > 0) {
_streams[streamId].snapshotDebtScaled += ongoingDebtScaled;
}
_streams[streamId].snapshotTime = uint40(block.timestamp);
_streams[streamId].ratePerSecond = newRatePerSecond;
}
Impact
The ability for the sender to unilaterally change the rate per second poses several risks, including:
-
Financial Loss: The recipient may find themselves in a situation where their income from the stream is dramatically reduced or eliminated entirely if the sender sets the rate to an undesirable value.
-
Trust Issues: The nature of the agreement between the sender and recipient may be compromised, eroding trust in the SablierFlow contract as a fair payment mechanism.
-
Exploitability: Malicious actors could exploit this vulnerability by creating contracts that manipulate the rate adjustment process, allowing for significant financial manipulation.
Tools Used
Manual review
Recommendations
To improve the security of the SablierFlow contract and mitigate the identified vulnerabilities, it is recommended to modify the _adjustRatePerSecond function to require approval from either the recipient or an admin before the sender can initiate any rate adjustments. This modification will ensure that all parties involved consent to any changes made. Below is the updated code along with tests to confirm that initiateRateAdjustment called by the sender and approveRateAdjustment work effectively together to enhance the protocol's security.
-
Adjusted code for SablierFlowcontract.
_streamRecipients;mapping was added for mapping recipient's approvals to streamId, this can also be done by adding the recipient to the Flow::Streamstreet in DataTypes
event RateAdjustmentInitiated(uint256 indexed streamId, UD21x18 newRatePerSecond);
event RateAdjustmentApproved(uint256 indexed streamId, UD21x18 newRatePerSecond);
struct RateAdjustmentRequest {
UD21x18 newRatePerSecond;
bool isPending;
}
mapping(uint256 => RateAdjustmentRequest) private _pendingRateAdjustments;
mapping(uint256 => address) private _streamRecipients;
modifier onlyRecipientOrAdmin(uint256 streamId) {
address recipient = _streamRecipients[streamId];
require(msg.sender == recipient || msg.sender == admin, "Not authorized: must be recipient or admin");
_;
}
function getPendingRateAdjustment(uint256 streamId)
external
view
returns (UD21x18 newRatePerSecond, bool isPending)
{
RateAdjustmentRequest memory request = _pendingRateAdjustments[streamId];
return (request.newRatePerSecond, request.isPending);
}
function initiateRateAdjustment(uint256 streamId, UD21x18 newRatePerSecond) external {
if(newRatePerSecond.unwrap() == _streams[streamId].ratePerSecond.unwrap()) {
revert Errors.SablierFlow_RatePerSecondNotDifferent(streamId, newRatePerSecond);
}
_pendingRateAdjustments[streamId] = RateAdjustmentRequest({
newRatePerSecond: newRatePerSecond,
isPending: true
});
emit RateAdjustmentInitiated(streamId, newRatePerSecond);
}
function approveRateAdjustment(uint256 streamId) external onlyRecipientOrAdmin(streamId) {
RateAdjustmentRequest storage request = _pendingRateAdjustments[streamId];
require(request.isPending, "No pending rate adjustments");
uint256 ongoingDebtScaled = _ongoingDebtScaledOf(streamId);
if (ongoingDebtScaled > 0) {
_streams[streamId].snapshotDebtScaled += ongoingDebtScaled;
}
_streams[streamId].snapshotTime = uint40(block.timestamp);
_streams[streamId].ratePerSecond = request.newRatePerSecond;
request.isPending = false;
emit RateAdjustmentApproved(streamId, request.newRatePerSecond);
}
Test code for the above functions
contract SablierFlowApprovalForAdjustmentOfRatePerSecondTest is Test {
SablierFlow public flow;
ERC20Mock token;
address public sender = address(0x1);
address public recipient = address(0x2);
address public initialAdmin = address(0x3);
UD21x18 initialRatePerSecond = UD21x18.wrap(1 ether);
UD21x18 newRatePerSecond = UD21x18.wrap(2 ether);
address public admin = address(0x3);
function setUp() public {
flow = new SablierFlow(initialAdmin, IFlowNFTDescriptor(address(0)));
token = new ERC20Mock("TestToken", "TTK", 18);
token.mint(sender, 100 ether);
vm.prank(sender);
token.approve(address(flow), 100 ether);
}
function testInitiateAndApproverateAdjustment() public {
uint256 streamId = flow.create(sender, recipient, initialRatePerSecond, IERC20(address(token)), true);
Flow.Stream memory stream = flow.getStream(streamId);
assertEq(stream.ratePerSecond.unwrap(), initialRatePerSecond.unwrap());
flow.initiateRateAdjustment(streamId, newRatePerSecond);
(, bool isPending) = flow.getPendingRateAdjustment(streamId);
assertTrue(isPending, "Rate adjustment should be pending");
vm.stopPrank();
vm.startPrank(admin);
flow.approveRateAdjustment(streamId);
stream = flow.getStream(streamId);
assertEq(stream.ratePerSecond.unwrap(), newRatePerSecond.unwrap(), "Rate per second should match the new rate");
(, isPending) = flow.getPendingRateAdjustment(streamId);
assertFalse(isPending, "No pending rate adjustment should remain after approval");
}
}
Adjustments to the createfunction because of the mapping created to map the recipient's approvals to streamId
function create(
address sender,
address recipient,
UD21x18 ratePerSecond,
IERC20 token,
bool transferable
)
external
override
noDelegateCall
returns (uint256 streamId)
{
// Checks, Effects, and Interactions: create the stream.
streamId = _create(sender, recipient, ratePerSecond, token, transferable);
+ _streamRecipients[streamId] = recipient;
+ return streamId;
}
