search

code-423n4 / 2022-10-holograph-findings

1 stars 0 forks source link

Missing support of non-standart ERC20 #477

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/enforcer/PA1D.sol#L317 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/enforcer/PA1D.sol#L340 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/HolographOperator.sol#L833 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/HolographOperator.sol#L740 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/HolographOperator.sol#L790

Vulnerability details

Vulnerability details

Description

In functions of PA1D and HolographOperator contracts there is logic relying on the fact that tokens implemented ERC20 standard (especially, that transfer and transferFrom functions of the tokens returns bool result). But in practice, many popular tokens do not have such equivalence and do not return any value in transfer and transferFrom functions. As an example, USDT has such ambiguity. In case of usage of such tokens, all statements of the form require(erc20.transfer(addresses[i], sending), "PA1D: Couldn't transfer token"); will revert because there will be a fail on decoding result into bool value.

Recommended Mitigation Steps

Use openzeppelin's safeTransfer and safeTransferFrom.

gzeoneth commented 1 year ago

Duplicate of #456

ACC01ADE commented 1 year ago

Using _callOptionalReturn to handle such edge-cases is a good suggestion.