search

GalloDaSballo / Apollon-Review

Notes for the Apollon Solo Security Review
0 stars 0 forks source link

Not using `safeTransfer` and `safeTransferFrom` can cause issues #36

Closed GalloDaSballo closed 2 months ago

GalloDaSballo commented 3 months ago

Impact

This is a very common bug that most of the times has no impact

However, at this time I don't have a complete list of tokens and implementations that will be used

Therefore it's best that you review this and decide whether it's acceptable to not use safeTransfer or whether you should use it everywhere

The code already uses safeTransfer in many places, leading me to believe it's best to use the same implementation everywhere

Not checking transfer return value will cause the system to break it's accounting

ERC20.transfer may not revert on failure, it may instead return false Not catching the return value can cause operations that should revert, not to revert

Using transfer on tokens that do not return a bool will revert

Some tokens like USDT do not return a bool on transfer

When you create an interface that specifies a bool return value, the solidity compiler will automatically add a check to ensure that the function returns a bool

This will make those calls revert, making the system incompatible with some (old) tokens

Instances

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9bed4b339e87/packages/contracts/contracts/BorrowerOperations.sol#L849-L850

    IERC20(_collAddress).transferFrom(_borrower, address(_pool), _amount); /// @audit FOT / SafeTransfer

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9bed4b339e87/packages/contracts/contracts/CollSurplusPool.sol#L69-L70

      IERC20(tokenEntry.tokenAddress).transfer(_account, tokenEntry.amount); /// @audit Use SafeTransfer

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9bed4b339e87/packages/contracts/contracts/ReservePool.sol#L100-L101

    govToken.transfer(stabilityPool, usedGov);

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9bed4b339e87/packages/contracts/contracts/ReservePool.sol#L104-L105

    stableDebtToken.transfer(stabilityPool, usedStable);

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9bed4b339e87/packages/contracts/contracts/StabilityPool.sol#L243-L244

    depositToken.transfer(user, depositToWithdrawal);

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9bed4b339e87/packages/contracts/contracts/StabilityPool.sol#L424-L425

      IERC20(collTokenAddress).transfer(_depositor, collGain);

https://github.com/blkswnStudio/ap/blob/8fab2b32b4f55efd92819bd1d0da9bed4b339e87/packages/contracts/contracts/StoragePool.sol#L110-L111

    IERC20(_tokenAddress).transfer(_receiver, _amount);

Mitigation

Use safeTransfer or add a comment in each line in which you chose not to use it

sambP commented 2 months ago

done