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

ERC-777 tokens are accepted violating protocol requirements

holydevoti0n

Summary

According to the documentation Sablier doesn't support ERC777 tokens.

Its not compatible with:
​
Any network which is not EVM compatible
@> **Any token standard other than ERC20**
Rebased ERC20 tokens can be used but yield will be lost
Ether (ETH)

Vulnerability Details

But currently there is no check that prevents a malicious user to add an ERC777 token. Besides, there is no check against reentrancy. Here's the function used to transfer funds when creating a stream from SablierV2BatchLockup:

function _handleTransfer(address sablierContract, IERC20 asset, uint256 amount) internal {
// Transfer the assets to the batchLockup contract.
asset.safeTransferFrom({ from: msg.sender, to: address(this), value: amount });
​
// Approve the Sablier contract to spend funds.
_approve(sablierContract, asset, amount);
}

The SafeERC20 doesn't prevent the use of ERC777. ERC777 has the same functions as a standard ERC20 token. Look at the snippet code below:

function transfer(address recipient, uint256 amount) external returns (bool) {
require(recipient != address(0), "ERC777: transfer to the zero address");
​
address from = msg.sender;
​
_callTokensToSend(from, from, recipient, amount, "", "");
​
_move(from, from, recipient, amount, "", "");
​
_callTokensReceived(from, from, recipient, amount, "", "", false);
​
return true;
}

As the contract doesn't have any check to prevent that, ERC777 tokens will be introduced in the system and will allow malicious users to use the hook _callTokensReceived when receiving those tokens.

Impact

  • ERC-777 is used but the system it is supposed to support only ERC20.

  • Hooks are introduced and malicious users has the possibility to use reentrancy as the contracts don't check for that.

Tools Used

Manual Review

Recommendations

A function could be add to check whether the token is an ERC777 or not. As the ERC777 token has specific functions that doesn't conform with the ERC20 standard like granularity and send for instance, this could be added as a safety check.

+function isERC777(address asset) internal returns (bool) {
+ try IERC777(asset).granularity() returns (uint) {
+ return true;
+ } catch {}
+ return false;
+ }
​
function _handleTransfer(address sablierContract, IERC20 asset, uint256 amount) internal {
+ if (isERC777(address(asset))) { revert Errors.SablierV2BatchLockup_TokenNotSupported() }
// Transfer the assets to the batchLockup contract.
asset.safeTransferFrom({ from: msg.sender, to: address(this), value: amount });
​
// Approve the Sablier contract to spend funds.
_approve(sablierContract, asset, amount);
}

Additionally, using the nonReentrant modifier from OZ could reinforce the safety against reentrancy.

Ref: https://docs.openzeppelin.com/contracts/2.x/api/token/erc777

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

Known - Contest Details

https://www.codehawks.com/contests/clvb9njmy00012dqjyaavpl44

holydevoti0n Submitter
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
holydevoti0n Submitter
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
holydevoti0n Submitter
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

Known - Contest Details

https://www.codehawks.com/contests/clvb9njmy00012dqjyaavpl44

Support

FAQs

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