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

clawback leads to loss of funds

mxusee

Description

clawback is implemented to retrieve unclaimed funds from a campaign when necessary, this can be useful in cases where something is not functioning as it should. as the docs state:

//In case of a malicious Merkle tree, `clawback` can be called to withdraw funds from the deployed `MerkleLockup` contracts until the grace period ends.

Keep in mind that this is merely one example of many situations where clawback can be invoked:

Inside function clawback, a check is performed for the grace period. This grace period is 7 days after the first claim has been made.

function clawback(address to, uint128 amount) external override onlyAdmin {
// Check: current timestamp is over the grace period and the campaign has not expired.
-> if (_hasGracePeriodPassed() && !hasExpired()) {
revert Errors.SablierV2MerkleLockup_ClawbackNotAllowed({
blockTimestamp: block.timestamp,
expiration: EXPIRATION,
firstClaimTime: _firstClaimTime
});
}
​
// Effect: transfer the tokens to the provided address.
ASSET.safeTransfer(to, amount);
​
// Log the clawback.
emit Clawback(admin, to, amount);
}

A recipient calls function claim to claim his funds which will later be unlocked for withdrawal.
Inside function claim, _checkClaim is called. Inside _checkClaim a check is performed to see if the claim is the first one made in the campaign.

function _checkClaim(uint256 index, bytes32 leaf, bytes32[] calldata merkleProof) internal {
// Check: the campaign has not expired.
if (hasExpired()) {
revert Errors.SablierV2MerkleLockup_CampaignExpired({
blockTimestamp: block.timestamp,
expiration: EXPIRATION
});
}
​
// Check: the index has not been claimed.
if (_claimedBitMap.get(index)) {
revert Errors.SablierV2MerkleLockup_StreamClaimed(index);
}
​
// Check: the input claim is included in the Merkle tree.
if (!MerkleProof.verify(merkleProof, MERKLE_ROOT, leaf)) {
revert Errors.SablierV2MerkleLockup_InvalidProof();
}
​
// Effect: set the `_firstClaimTime` if its zero.
-> if (_firstClaimTime == 0) {
_firstClaimTime = uint40(block.timestamp);
}
}

When the recipient calls claim, a stream is created, and the funds are transferred to the core contracts.

function _create(LockupLinear.CreateWithTimestamps memory params) internal returns (uint256 streamId) {
//..Ommitted code
​
// Effect: bump the next stream ID.
// Using unchecked arithmetic because these calculations cannot realistically overflow, ever.
unchecked {
nextStreamId = streamId + 1;
}
​
// Effect: mint the NFT to the recipient.
_mint({ to: params.recipient, tokenId: streamId });
​
// Interaction: transfer the deposit amount.
-> params.asset.safeTransferFrom({ from: msg.sender, to: address(this), value: createAmounts.deposit });
//..Ommitted code
}

Furthermore, the team confirms the following, as per the discord message:

// Q: If an airstream is created and a user calls claim can that amount still be retrieved by clawback ? Or is the clawback specifically made to retrieve amounts from users that have not called claim yet?
// A: No. Clawback can only be applied on the unclaimed funds. When a user claims, a stream is created, which transfers funds to the core contracts.

This means that if clawback is called, it will no longer be able to retrieve these funds. It can only retrieve funds from a recipient who has not yet called claim, since ultimately calling claim results in funds being transferred.

Impact

Considering all of this, the following scenario will always occur whenever clawback is called after the _firstClaim has been set:

  • Bob is the first person in his campaign to call claim.

  • _firstClaimTime is set to the block.timestamp of Bob's call.

  • The admin notices an issue with the campaign, which, according to the docs, "can result in "potentially leading to unclaimable assets or unexpected behavior."

  • The admin calls clawback, but since Bob has already called claim, his funds have been transferred to the core contracts.

  • clawback will be unable to retrieve Bob's funds.

  • Bob loses his funds.

There is another scenario that does not require a _FirstClaim to be made:

  • Alice is a malicious recipient.

  • Alice notices clawback being called and frontruns it by calling claim.

  • The admin will no longer be able to retrieve her funds.

  • Alice can either safely withdraw her funds later, or, the funds become stuck and the stream sender loses those funds.

As we can see, whether the environment is honest or dishonest, this will result in a loss of funds, hence the HIGH label.

Tools Used

Manual Review

Recommendation

allow clawback to retrieve the funds inside the core contracts.

Updates

Lead Judging Commences

inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
mxusee Submitter
over 1 year ago
inallhonesty Lead Judge
over 1 year ago
mxusee Submitter
over 1 year ago
inallhonesty Lead Judge over 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.