Flow
Sablier
FoundryDeFi
20,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

It is possible to `depositViaBroker` to a stream while `broker` address is the same as `recipient` or the `sender`

ljj

Summary

There are no checks implemented to make sure that the broker address is not the same as the recipient or the sender address when calling the depositViaBroker function on a stream.

Vulnerability Details

The depositViaBroker function allows the depositors to pay a certain fee to the broker while depositing into the stream.

function depositViaBroker(
uint256 streamId,
uint128 totalAmount,
address sender,
address recipient,
Broker calldata broker
)
external
override
noDelegateCall
notNull(streamId)
notVoided(streamId)
updateMetadata(streamId)
{
// Check: the provided sender and recipient match the stream's sender and recipient.
_verifyStreamSenderRecipient(streamId, sender, recipient);
​
// Checks, Effects, and Interactions: deposit on stream through broker.
_depositViaBroker(streamId, totalAmount, broker);
}
function _depositViaBroker(uint256 streamId, uint128 totalAmount, Broker memory broker) internal {
// Check: verify the `broker` and calculate the amounts.
(uint128 brokerFeeAmount, uint128 depositAmount) =
Helpers.checkAndCalculateBrokerFee(totalAmount, broker, MAX_FEE);
​
// Checks, Effects, and Interactions: deposit on stream.
_deposit(streamId, depositAmount);
​
// Interaction: transfer the broker's amount.
_streams[streamId].token.safeTransferFrom({ from: msg.sender, to: broker.account, value: brokerFeeAmount });
}

Accoring to the comments in the ISablierFlow.sol interface contract, a broker is defined as:

// redacted for clarity
/// @param broker Struct encapsulating (i) the address of the broker assisting in creating the stream, and (ii) the
/// percentage fee paid to the broker from `totalAmount`, denoted as a fixed-point percentage.
function depositViaBroker(
uint256 streamId,
uint128 totalAmount,
address sender,
address recipient,
Broker calldata broker
)
external;

Therefor, it makes no sense for the recipient or the sender to be the broker of a given stream as they are participating actors in the stream instead of being an actor that assists in creating the stream. However, lack of checks allow these addresses to also be the broker of the stream. This makes it possible for depositors to set the broker address as either the sender or the recipient address, breaking the logic of having a broker in the first place. This vulnerability makes it easier to:
1) Have accidental inputs to set broker as the recipient or the sender as they are also inputting these addresses when depositing.

2) Maliciously entering the recipient or the sender as the broker address to bypass the fees that should be given to the broker and acting like this is a mistake. As the depositors can not refund their deposited amount, they can't refund and deposit again with the correct broker address.

Keep in mind that there are no trust relationship between the depositors and the broker according to the Known Issues section.

It is assumed that a trust relationship is formed between the sender, recipient, and depositors participating in a stream.

Proof of Concept

Add the following test contract in the tests file and run it to observe the vulnerability. In order to run this test, add a mint function in the MockERC20.sol

pragma solidity >=0.8.22;
​
import { Test } from "forge-std/src/Test.sol";
import { console } from "forge-std/src/console.sol";
import "../src/SablierFlow.sol";
import "../src/interfaces/IFlowNFTDescriptor.sol";
import "./mocks/ERC20Mock.sol";
​
// Mock NFT descriptor to satisfy constructor requirements
contract MockNFTDescriptor is IFlowNFTDescriptor {
function tokenURI(IERC721Metadata, uint256) external pure override returns (string memory) {
return "test_uri";
}
}
​
contract CustomTest is Test {
SablierFlow private flow;
ERC20Mock private token;
ERC20Mock private wbtc;
MockNFTDescriptor private nftDescriptor;
​
address private sender = address(0x1);
address private recipient = address(0x2);
address private admin = address(0x3);
address private depositor = address(0x4);
address private brokerA = address(0x5);
uint128 private amount = 200e8;
uint256 private streamId;
​
function setUp() public {
// Deploy mock token with 18 decimals and NFT descriptor
token = new ERC20Mock("MockToken", "MTK", 0);
wbtc = new ERC20Mock("Wrapped Bitcoin", "WBTC", 8);
nftDescriptor = new MockNFTDescriptor();
​
// Mint initial tokens directly to the depositor's address
token.mint(depositor, amount);
wbtc.mint(depositor, amount);
​
// Deploy the SablierFlow contract
flow = new SablierFlow(address(admin), nftDescriptor);
​
// Approve the SablierFlow contract to spend the depositor's tokens
vm.startPrank(depositor);
token.approve(address(flow), amount);
wbtc.approve(address(flow), amount);
vm.stopPrank();
}
function testDepositViaBroker() public {
// Set up broker as the stream recipient
Broker memory broker = Broker({ account: recipient, fee: UD60x18.wrap(0.1e18) });
​
// cached values
uint128 depositAmount = 200;
uint256 brokerFee = (depositAmount * 10) / 100;
​
// call depositViaBroker
vm.prank(sender);
uint256 streamId = flow.create(sender, recipient, ud21x18(1e1), token, true);
vm.prank(depositor);
flow.depositViaBroker(streamId, depositAmount, sender, recipient, broker);
uint256 recipientBalance = token.balanceOf(recipient);
// assertion
assertEq(recipientBalance, brokerFee);
}

Impact

Impact: Medium as it makes it easier to maliciously or accidentally bypass the broker
Likelihood: Low

Tools Used

Manual review, foundry

Recommendations

Implement checks to make sure that the sender or the recipient address can not be the broker address.
Alternatively, in the stream creation, make the sender set the broker address and only allow depositViaBroker to be used for deposits. Use the address set at the stream creation instead of taking user input for this address.
An example fix is shown below.

function _depositViaBroker(uint256 streamId, uint128 totalAmount, Broker memory broker) internal {
// Check: verity the `broker` address is not the `sender` or the `recipient` address.
if (broker.account == _ownerOf(streamId) || broker.account == _streams[streamId].sender,){
revert Errors.SablierFlow_InvalidAddress(broker.account);
}
// rest of the function
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.