Wrong parameter when retrieving causes a complete DoS of the protocol
A wrong parameter in the `_retrieve()` prevents the protocol from properly interacting with Sablier, causing a Denial of Service in all functions calling `_retrieve()`.\\nThe `CouncilMember` contract is designed to interact with a Sablier stream. As time passes, the Sablier stream will unlock more TELCOIN tokens which will be available to be retrieved from `CouncilMember`.\\nThe `_retrieve()` internal function will be used in order to fetch the rewards from the stream and distribute them among the Council Member NFT holders (snippet reduced for simplicity):\\n```\\n// CouncilMember.sol\\n\\nfunction _retrieve() internal {\\n        // rest of code\\n        // Execute the withdrawal from the _target, which might be a Sablier stream or another protocol\\n        _stream.execute(\\n            _target,\\n            abi.encodeWithSelector(\\n                ISablierV2ProxyTarget.withdrawMax.selector, \\n                _target, \\n                _id,\\n                address(this)\\n            )\\n        );\\n\\n        // rest of code\\n    }\\n```\\n\\nThe most important part in `_retrieve()` regarding the vulnerability that we'll dive into is the `_stream.execute()` interaction and the params it receives. In order to understand such interaction, we first need understand the importance of the `_stream` and the `_target` variables.\\nSablier allows developers to integrate Sablier via Periphery contracts, which prevents devs from dealing with the complexity of directly integrating Sablier's Core contracts. Telcoin developers have decided to use these periphery contracts. Concretely, the following contracts have been used:\\nNOTE: It is important to understand that the actual lockup linear stream will be deployed as well. The difference is that the Telcoin protocol will not interact with that contract directly. Instead, the PRBProxy and proxy target contracts will be leveraged to perform such interactions.\\nKnowing this, we can now move on to explaining Telcoin's approach to withdrawing the available tokens from the stream. As seen in the code snippet above, the `_retrieve()` function will perform two steps to actually perform a withdraw from the stream:\\nIt will first call the _stream's `execute()` function (remember `_stream` is a PRBProxy). This function receives a `target` and some `data` as parameter, and performs a delegatecall aiming at the target:\\nIn the `_retrieve()` function, the target where the call will be forwarded to is the `_target` parameter, which is a ProxyTarget contract. Concretely, the delegatecall function that will be triggered in the ProxyTarget will be withdrawMax():\\nAs we can see, the `withdrawMax()` function has as parameters the `lockup` stream contract `to` withdraw from, the `streamId` and the address `to` which will receive the available funds from the stream. The vulnerability lies in the parameters passed when calling the `withdrawMax()` function in `_retrieve()`. As we can see, the first encoded parameter in the `encodeWithSelector()` call after the selector is the _target:\\n```\\n// CouncilMember.sol\\n\\nfunction _retrieve() internal {\\n        // rest of code\\n        // Execute the withdrawal from the _target, which might be a Sablier stream or another protocol\\n        _stream.execute(\\n            _target,\\n            abi.encodeWithSelector(\\n                ISablierV2ProxyTarget.withdrawMax.selector, \\n                _target,   // <------- This is incorrect\\n                _id,\\n                address(this)\\n            )\\n        );\\n\\n        // rest of code\\n    }\\n```\\n\\nThis means that the proxy target's `withdrawMax()` function will be triggered with the `_target` contract as the `lockup` parameter, which is incorrect. This will make all calls eventually execute `withdrawMax()` on the PRBProxy contract, always reverting.\\nThe parameter needed to perform the `withdrawMax()` call correctly is the actual Sablier lockup contract, which is currently not stored in the `CouncilMember` contract.\\nThe following diagram also summarizes the current wrong interactions for clarity:
In order to fix the vulnerability, the proper address needs to be passed when calling `withdrawMax()`.\\nNote that the actual stream address is currently NOT stored in `CouncilMember.sol`, so it will need to be stored (my example shows a new `actualStream` variable)\\n```\\nfunction _retrieve() internal {\\n        // rest of code\\n        // Execute the withdrawal from the _target, which might be a Sablier stream or another protocol\\n        _stream.execute(\\n            _target,\\n            abi.encodeWithSelector(\\n                ISablierV2ProxyTarget.withdrawMax.selector, \\n// Remove the line below\\n                _target, \\n// Add the line below\\n  actualStream\\n                _id,\\n                address(this)\\n            )\\n        );\\n\\n        // rest of code\\n    }\\n```\\n
High. ALL withdrawals from the Sablier stream will revert, effectively causing a DoS in the _retrieve() function. Because the _retrieve() function is called in all the main protocol functions, this vulnerability essentially prevents the protocol from ever functioning correctly.\\nProof of Concept\\n```\\n// SPDX-License-Identifier: UNLICENSED\\npragma solidity ^0.8.13;\\n\\nimport {Test, console2} from "forge-std/Test.sol";\\nimport {SablierV2Comptroller} from "@sablier/v2-core/src/SablierV2Comptroller.sol";\\nimport {SablierV2NFTDescriptor} from "@sablier/v2-core/src/SablierV2NFTDescriptor.sol";\\nimport {SablierV2LockupLinear} from "@sablier/v2-core/src/SablierV2LockupLinear.sol";\\nimport {ISablierV2Comptroller} from "@sablier/v2-core/src/interfaces/ISablierV2Comptroller.sol";\\nimport {ISablierV2NFTDescriptor} from "@sablier/v2-core/src/interfaces/ISablierV2NFTDescriptor.sol";\\nimport {ISablierV2LockupLinear} from "@sablier/v2-core/src/interfaces/ISablierV2LockupLinear.sol";\\n\\nimport {CouncilMember, IPRBProxy} from "../src/core/CouncilMember.sol";\\nimport {TestTelcoin} from "./mock/TestTelcoin.sol";\\nimport {MockProxyTarget} from "./mock/MockProxyTarget.sol";\\nimport {PRBProxy} from "./mock/MockPRBProxy.sol";\\nimport {PRBProxyRegistry} from "./mock/MockPRBProxyRegistry.sol";\\n\\nimport {UD60x18} from "@prb/math/src/UD60x18.sol";\\nimport {LockupLinear, Broker, IERC20} from "@sablier/v2-core/src/types/DataTypes.sol";\\nimport {IERC20 as IERC20OZ} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";\\n\\ncontract PocTest is Test {\\n\\n    ////////////////////////////////////////////////////////////////\\n    //                        CONSTANTS                           //\\n    ////////////////////////////////////////////////////////////////\\n\\n   bytes32 public constant GOVERNANCE_COUNCIL_ROLE =\\n        keccak256("GOVERNANCE_COUNCIL_ROLE");\\n    bytes32 public constant SUPPORT_ROLE = keccak256("SUPPORT_ROLE");\\n\\n    ////////////////////////////////////////////////////////////////\\n    //                         STORAGE                            //\\n    ////////////////////////////////////////////////////////////////\\n\\n    /// @notice Poc Users\\n    address public sablierAdmin;\\n    address public user;\\n\\n    /// @notice Sablier contracts\\n    SablierV2Comptroller public comptroller;\\n    SablierV2NFTDescriptor public nftDescriptor;\\n    SablierV2LockupLinear public lockupLinear;\\n\\n    /// @notice Telcoin contracts\\n    PRBProxyRegistry public proxyRegistry;\\n    PRBProxy public stream;\\n    MockProxyTarget public target;\\n    CouncilMember public councilMember;\\n    TestTelcoin public telcoin;\\n\\n    function setUp() public {\\n        // Setup users\\n        _setupUsers();\\n\\n        // Deploy token\\n        telcoin = new TestTelcoin(address(this));\\n\\n        // Deploy Sablier \\n        _deploySablier();\\n\\n        // Deploy council member\\n        councilMember = new CouncilMember();\\n\\n        // Setup stream\\n        _setupStream();\\n\\n        // Setup the council member\\n        _setupCouncilMember();\\n    }\\n\\n    function testPoc() public {\\n      // Step 1: Mint council NFT to user\\n      councilMember.mint(user);\\n      assertEq(councilMember.balanceOf(user), 1);\\n\\n      // Step 2: Forward time 1 days\\n      vm.warp(block.timestamp + 1 days);\\n      \\n      // Step 3: All functions calling _retrieve() (mint(), burn(), removeFromOffice()) will fail\\n      vm.expectRevert(abi.encodeWithSignature("PRBProxy_ExecutionReverted()")); \\n      councilMember.mint(user);\\n    }\\n\\n    function _setupUsers() internal {\\n        sablierAdmin = makeAddr("sablierAdmin");\\n        user = makeAddr("user");\\n    }\\n\\n    function _deploySablier() internal {\\n        // Deploy protocol\\n        comptroller = new SablierV2Comptroller(sablierAdmin);\\n        nftDescriptor = new SablierV2NFTDescriptor();\\n        lockupLinear = new SablierV2LockupLinear(\\n            sablierAdmin,\\n            ISablierV2Comptroller(address(comptroller)),\\n            ISablierV2NFTDescriptor(address(nftDescriptor))\\n        );\\n    }\\n\\n    function _setupStream() internal {\\n\\n        // Deploy proxies\\n        proxyRegistry = new PRBProxyRegistry();\\n        stream = PRBProxy(payable(address(proxyRegistry.deploy())));\\n        target = new MockProxyTarget();\\n\\n        // Setup stream\\n        LockupLinear.Durations memory durations = LockupLinear.Durations({\\n            cliff: 0,\\n            total: 1 weeks\\n        });\\n\\n        UD60x18 fee = UD60x18.wrap(0);\\n\\n        Broker memory broker = Broker({account: address(0), fee: fee});\\n        LockupLinear.CreateWithDurations memory params = LockupLinear\\n            .CreateWithDurations({\\n                sender: address(this),\\n                recipient: address(stream),\\n                totalAmount: 100e18,\\n                asset: IERC20(address(telcoin)),\\n                cancelable: false,\\n                transferable: false,\\n                durations: durations,\\n                broker: broker\\n            });\\n\\n        bytes memory data = abi.encodeWithSelector(target.createWithDurations.selector, address(lockupLinear), params, "");\\n\\n        // Create the stream through the PRBProxy\\n        telcoin.approve(address(stream), type(uint256).max);\\n        bytes memory response = stream.execute(address(target), data);\\n        assertEq(lockupLinear.ownerOf(1), address(stream));\\n    }\\n\\n    function _setupCouncilMember() internal {\\n      // Initialize\\n      councilMember.initialize(\\n            IERC20OZ(address(telcoin)),\\n            "Test Council",\\n            "TC",\\n            IPRBProxy(address(stream)), // stream_\\n            address(target), // target_\\n            1, // id_\\n            address(lockupLinear)\\n        );\\n\\n        // Grant roles\\n        councilMember.grantRole(GOVERNANCE_COUNCIL_ROLE, address(this));\\n        councilMember.grantRole(SUPPORT_ROLE, address(this));\\n    }\\n  \\n}\\n```\\n
```\\n// CouncilMember.sol\\n\\nfunction _retrieve() internal {\\n        // rest of code\\n        // Execute the withdrawal from the _target, which might be a Sablier stream or another protocol\\n        _stream.execute(\\n            _target,\\n            abi.encodeWithSelector(\\n                ISablierV2ProxyTarget.withdrawMax.selector, \\n                _target, \\n                _id,\\n                address(this)\\n            )\\n        );\\n\\n        // rest of code\\n    }\\n```\\n