search

sherlock-audit / 2023-03-teller-judging

8 stars 6 forks source link

ak1 - CollateralEscrowV1.sol : `_withdrawCollateral` , funds would be lost the some collateral that fails with false return, #510

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

ak1

medium

CollateralEscrowV1.sol : _withdrawCollateral , funds would be lost the some collateral that fails with false return,

Summary

During withdraw process, _withdrawCollateral is called and funds are trnasferred using the either transfer or transferFrom or safeTransferFrom functions.

The contract is using the regular transfer call for ERC20 tokens.

Vulnerability Detail

function _withdrawCollateral(
    Collateral memory _collateral,
    address _collateralAddress,
    uint256 _amount,
    address _recipient
) internal {
    // Withdraw ERC20
    if (_collateral._collateralType == CollateralType.ERC20) {
        IERC20Upgradeable(_collateralAddress).transfer(
            _recipient,
            _collateral._amount
        );
    }
    // Withdraw ERC721
    else if (_collateral._collateralType == CollateralType.ERC721) {
        require(_amount == 1, "Incorrect withdrawal amount");
        IERC721Upgradeable(_collateralAddress).transferFrom(
            address(this),
            _recipient,
            _collateral._tokenId
        );
    }

Note : safeERC20 operation is not invoked, through the contract safeERC20 is inherited.

Impact

Fund would be lost incase the collateral returns false in case of failure.

Code Snippet

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/escrow/CollateralEscrowV1.sol#L158-L179

Tool used

Manual Review

Recommendation

 */
function _withdrawCollateral(
    Collateral memory _collateral,
    address _collateralAddress,
    uint256 _amount,
    address _recipient
) internal {
    // Withdraw ERC20
    if (_collateral._collateralType == CollateralType.ERC20) {
        IERC20Upgradeable(_collateralAddress).transfer(
            _recipient,
            _collateral._amount
        ); -------------------------------------------------------------- >from adit - remove

      require(IERC20Upgradeable(_collateralAddress).transfer(
            _recipient,
            _collateral._amount
        )); ---------------------------------------------------------------> from audit - add
    }
    // Withdraw ERC721

Duplicate of #220