Transfer returned value is not checked in `LibStaking.crvPriceReset()`

[0xRizwan]

Title

Transfer returned value is not checked in LibStaking.crvPriceReset()

Severity

Medium

Vulnerability details

LibStaking.crvPriceReset() is used to remove 3CRV unilaterally (ts.token1) from the curve LP share sitting inside the staking contract and send the 3CRV received to the treasury address.

    function crvPriceReset(uint256 amount) internal {
        LibTWAPOracle.TWAPOracleStorage memory ts = LibTWAPOracle
            .twapOracleStorage();
        IMetaPool metaPool = IMetaPool(ts.pool);
        // remove one coin
        uint256 coinWithdrawn = metaPool.remove_liquidity_one_coin(
            amount,
            1,
            0
        );
        // update twap
        LibTWAPOracle.update();
        uint256 toTransfer = IERC20(ts.token1).balanceOf(address(this));

        IERC20(ts.token1).transfer(                              @audit // transfer value is unchecked for USDT like tokens
            LibAppStorage.appStorage().treasuryAddress,
            toTransfer
        );
        emit PriceReset(ts.token1, coinWithdrawn, toTransfer);
    }

The issue here is with the use of unsafe transfer() function. Curve tri-pool supports DAI, USDC, USDT stablecoins and this issue is about non-standard behavior of USDT.

The ERC20.transfer() function return a boolean value indicating success. This parameter needs to be checked for success. Per EIP20. transfer() function is given below,

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

and USDT transfer() function looks as below,

    function transfer(address _to, uint _value) public whenNotPaused {

Therefore, tokens (like USDT) don't correctly implement the EIP20 standard and their transfer() function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. as USDT transfer do not revert if the transfer failed.

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer and tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the protocol as they revert the transaction because of the missing return value.

Impact

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer and tokens that don't correctly implement the latest EIP20 spec will be unusable in the protocol as they revert the transaction because of the missing return value.

Recommendation

Recommend using safeTransfer() function from already imported OpenZeppelin's SafeERC20.sol that handle the return value check as well as non-standard-compliant tokens.

    function crvPriceReset(uint256 amount) internal {
        LibTWAPOracle.TWAPOracleStorage memory ts = LibTWAPOracle
            .twapOracleStorage();
        IMetaPool metaPool = IMetaPool(ts.pool);
        // remove one coin
        uint256 coinWithdrawn = metaPool.remove_liquidity_one_coin(
            amount,
            1,
            0
        );
        // update twap
        LibTWAPOracle.update();
        uint256 toTransfer = IERC20(ts.token1).balanceOf(address(this));

-        IERC20(ts.token1).transfer(                                                               
-            LibAppStorage.appStorage().treasuryAddress,
-            toTransfer
-        );
+        IERC20(ts.token1).safeTransfer(                                                               
+            LibAppStorage.appStorage().treasuryAddress,
+            toTransfer
+        );
        emit PriceReset(ts.token1, coinWithdrawn, toTransfer);
    }

cc- @pavlovcik @molecula451

[molecula451]

I understand that you want to provide value with a research, a bit in-depth analysis to the codebase and accomplish rewards + take part on the repo, this is great, but it's better job to please take time to asses the codebase a bit further.

We already implement most of this standard security compliants by OpenZeppelin on the repo

https://github.com/ubiquity/ubiquity-dollar/blob/38c3656539ae19fe9be162f566b36ec62a3e6e41/packages/contracts/src/dollar/libraries/LibStaking.sol#L13

[ubiquibot[bot]]

# Issue was not closed as completed. Skipping.

[0xRizwan]

@molecula451 @pavlovcik

I have checked it. transfer() should be replaced with safeTransfer() which is from already imported safeERC20.sol.

[0x4007]

@rndquu rfc

[molecula451]

I have checked it. transfer() should be replaced with safeTransfer() which is from already imported safeERC20.sol.

There is no need to do when we globally use SafeERC20 for all IERC20 interfaces solidity automatically enables this to the state interface e.g IERC20, so all .transfer() are basically .safeTransfer()

[rndquu]

1. I agree that transfer() call results should be checked
2. ts.token1 is not an externally controllable param. It is set during deployment by an admin so the issue is of low severity.
3. Staking contracts are really outdated, we didn't even include them in the audit since they should be refactored

I would close the current issue as "not planned"