Royalties cannot be collected for many ERC20 tokens (USDT, BNB and many more) due to use of transfer function.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L416
https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L439

Vulnerability details

Description

ERC20 royalties are paid using _payoutTokens and _payoutToken functions in PA1D.sol. Unfortunately these functions use ERC20's transfer() instead of implementing safeTransfer:

    for (uint256 i = 0; i < length; i++) {
      sending = ((bps[i] * balance) / 10000);
      require(erc20.transfer(addresses[i], sending), "PA1D: Couldn't transfer token");
      // sent = sent + sending;
    }

The transfer signature includes a bool return value:

function transfer(address _to, uint256 _value) external returns (bool success);

Therefore, the calling code discovers there is no bool return parameter, it will revert. The impact is any non conforming ERC20 tokens will be forever stuck in the Holographer contract. There are hundreds of such tokens, such as USDT, BNB etc.

Because of the likelihood of non-conforming ERC20 tokens being a royalty token, this represents a high severity threat.

Impact

USDT, BNB and any other no-return-value coin will be stuck in the Holographer contract.

Tools Used

Manual audit

Recommended Mitigation Steps

Use OpenZeppelin's SafeERC20 library to interact with ERC20 tokens.

[d3e4]

Duplicate of #456

[gzeoneth]

Duplicate of #456

[ACC01ADE]

Other exact same issue submissions have a maximum of 2 Med Risk severity, this one has to be same as the rest.