Sablier
Sablier
DeFiFoundry
53,440 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Lack of Explicit Checks for Approval of Non-Existent Owners or Operators in the SablierV2Lockup contract

lordofterra

Summary

The cancel, burn, and withdraw functions rely on the _isCallerStreamRecipientOrApproved function to verify whether the caller (msg.sender) is authorized to perform the action. However, _isCallerStreamRecipientOrApproved does not explicitly check if the recipient (owner) or the operator exists. This oversight can lead to scenarios where approvals are set for non-existent addresses, causing potential security risks.

Proof of concept

  1. Checking approval for a non-existent owner.

function testNonExistentOwnerApproval() public {
address nonExistentOwner = address(0x123); // Assuming this address has no tokens
address operator = address(this);
​
// Expecting false since the owner does not exist
bool isApproved = contractInstance.isApprovedForAll(nonExistentOwner, operator);
assert(!isApproved);
}

  1. Checking approval for a non-existent operator.

function testNonExistentOperatorApproval() public {
address owner = address(0x456); // Assuming this address owns some tokens
address nonExistentOperator = address(0x789); // Assuming this address is not an operator
​
// Expecting false since the operator does not exist
bool isApproved = contractInstance.isApprovedForAll(owner, nonExistentOperator);
assert(!isApproved);
}

  1. Checking approval for zero address as owner or operator.

function testZeroAddressApproval() public {
address zeroAddress = address(0);
address operator = address(this);
​
// Expecting false since the owner is the zero address
bool isApprovedForZeroOwner = contractInstance.isApprovedForAll(zeroAddress, operator);
assert(!isApprovedForZeroOwner);
​
address owner = address(0x456); // Assuming this address owns some tokens
​
// Expecting false since the operator is the zero address
bool isApprovedForZeroOperator = contractInstance.isApprovedForAll(owner, zeroAddress);
assert(!isApprovedForZeroOperator);
}

Impact

  1. The function may return false for non-existent addresses, which is generally safe but can lead to confusion or unexpected behavior.

  2. If the contract logic assumes the existence of an owner or operator without validation, it could potentially be exploited in edge cases or during complex interactions.

Tools Used

Manual review

Recommendations

  1. Implement explicit checks to ensure that both the owner and operator addresses exist before performing approval checks. This can be done by verifying that the addresses are not zero and that they are valid within the context of the contract.

Here is an example function that implements the various checks

function isApprovedForAll(address owner, address operator) public view virtual override returns (bool) {
require(owner != address(0), "Owner address is zero");
require(operator != address(0), "Operator address is zero");
return _operatorApprovals[owner][operator];
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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