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

LOWs

touthang

[L-01]

Summary

Protocol supports creating streams with any ERC20 tokens(except FoT and rebase tokens) but there can be calculation issues incase of tokens with extremely low decimals.

Vulnerability Details

Example, tokens like Gemini USD have only 2 decimals, in that case suppose a broker charge a fee of 0.01% for every stream created through it, then the broker will not get any fees when users create streams with small amount, any users creating with less than 100e2 tokens will not give any fees to the broker because checkAndCalculateBrokerFee will return 0 for the amount.brokerFee since it will round down to 0.
amounts.brokerFee = uint128(ud(totalAmount).mul(brokerFee).intoUint256());

function checkAndCalculateBrokerFee(
uint128 totalAmount,
UD60x18 brokerFee,
UD60x18 maxBrokerFee
)
internal
pure
returns (Lockup.CreateAmounts memory amounts)
{
// When the total amount is zero, the broker fee is also zero.
if (totalAmount == 0) {
return Lockup.CreateAmounts(0, 0);
}
​
// Check: the broker fee is not greater than `maxBrokerFee`.
if (brokerFee.gt(maxBrokerFee)) {
revert Errors.SablierV2Lockup_BrokerFeeTooHigh(brokerFee, maxBrokerFee);
}
​
// Calculate the broker fee amount.
// The cast to uint128 is safe because the maximum fee is hard coded.
amounts.brokerFee = uint128(ud(totalAmount).mul(brokerFee).intoUint256());
​
// Assert that the total amount is strictly greater than the broker fee amount.
assert(totalAmount > amounts.brokerFee);
​
// Calculate the deposit amount (the amount to stream, net of the broker fee).
amounts.deposit = totalAmount - amounts.brokerFee;
}

Impact

broker fails to charge fees for their service.

The impact will also depend on the ratio between how much fee is charged and how much it is deposited. The less the fee percentage the more larger the amount of deposits that can be the without paying fees.

Tools Used

manual

Recommendations

Try checking the decimals of the tokens when creating a new stream and revert if less than 6. Or Brokers should be notified here that atleast 1% fee should be charged for low decimal tokens.

[L-02]

Vulnerability Details

Protocol uses Adminable contract which is inherited by other contracts of the protocol, where the admin can change the admin with the transferAdmin function.

However, if the admin is changed incorrectly by entering the wrong address, it cannot be taken back, it allows the owner to transfer ownership to a non-existent or mistyped address.

function transferAdmin(address newAdmin) public virtual override onlyAdmin {
// Effect: update the admin.
admin = newAdmin;
​
// Log the transfer of the admin.
emit IAdminable.TransferAdmin({ oldAdmin: msg.sender, newAdmin: newAdmin });
}

Impact

  • core - SablierV2LockupLinear, SablierV2LockupTranched and SablierV2LockupDynamic

  • periphery - SablierV2MerkleLL and SablierV2MerkleLT

These contracts will be left with no admin and only admin functions are lost.
Incase of the periphery contracts the likelihood increases.

Tools Used

manual

Recommendations

Use Ownable2Step from openzeppelin which instead of directly transferring to the new admin, the transfer only completes when the new admin accepts ownership.

function acceptOwnership() public virtual {
address sender = _msgSender();
if (pendingOwner() != sender) {
revert OwnableUnauthorizedAccount(sender);
}
_transferOwnership(sender);
}

Ownable2Step should be implemented especially for the SablierV2MerkleLockup because in this case the users(campaign creators) are the admin.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Info/Gas/Invalid as per Docs

https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity

Support

FAQs

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