Yieldy contest - GalloDaSballo's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L769

Vulnerability details

The function preSign acceps any orderUid function preSign(bytes calldata orderUid) external onlyOwner

Because of how Cowswap works, accepting any orderUid can be used as a rug-vector.

This is because the orderData contains a receiver which in lack of validation could be any address.

You'd also be signing other parameters such as minOut and how long the order could be filled for, which you may or may not want to validate to give stronger security guarantees to end users.

Recomended mitigation steps

I'd recommend adding basic validation for tokenOut, minOut and receiver.

Feel free to check the work we've done at Badger to validate order parameters, giving way stronger guarantees to end users. https://github.com/GalloDaSballo/fair-selling/blob/44c0c0629289a0c4ccb3ca971cc5cd665ce5cb82/contracts/CowSwapSeller.sol#L194

Also notice how through the code above we are able to re-construct the orderUid, feel free to re-use that code which has been validated by the original Cowswap / GPv2 Developers

Executive Summary

Overall the codebase is well documented and easy to read.

It could benefit with some gas optimizations and unfortunately the role based model will always open up to a less trusted setup because of how open-ended it is

Beside that the code can benefit via the following

Suggestion - Cowswap, no way to delete pending orders

preSign allows the admin to place an order, but not cancel it.

    function preSign(bytes calldata orderUid) external onlyOwner {
        ICowSettlement(COW_SETTLEMENT).setPreSignature(orderUid, true);
    }

From my actual experience, some orders can be stuck and some can fail, I highly recommend adding a way to setPresignature(orderUid, false) to avoid any possible problem

Suggestion - Refactor function to use one less parameters

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L741-L745

    function addRewardsForStakers(
        uint256 _amount,
        bool _shouldTransfer,
        bool _trigger
    ) external {

Can be refactored to check _amount, if _amount > 0 perform the transfer, else ignore

Change

        if (_shouldTransfer) {
            IERC20Upgradeable(STAKING_TOKEN).safeTransferFrom(
                msg.sender,
                address(this),
                _amount
            );
        }

To

        if (_amount > 0) {
            IERC20Upgradeable(STAKING_TOKEN).safeTransferFrom(
                msg.sender,
                address(this),
                _amount
            );
        }

And remove _shouldTransfer

Non-Critical - Variable that can changed ALL_CAPSED

By convention, the codebase is using ALL_CAPS for Constants / Immutables , yet FEE_ADDRESS is ALL_CAPS and has a setter.

I'd recommend camelCasing it.

-FEE_ADDRESS

Also valid for:

• CURVE_POOL

Non-Critical - No check for duplicate

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L77-L83

    /**
        @notice add address to contracts array
        @param _address - address to add
     */
    function addAddress(address _address) external onlyOwner {
        contracts.push(_address);
    }

It may not be ideal to allow duplicate submissions to be added

Deletion however seems to be fine at handling duplicates

Executive Summary

Below are listed gas savings refactorings along with the estimated gas saved.

Reducing SLOADS is by far the best way to save gas on this codebase, for that reason most of the advice is based on that

Minor gas savings are also listed, but reducing SLOADs should be prioritized especially as it will save gas for end users

Optimized setCurvePool - 4400 gas

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L157-L158

    function setCurvePool(address _curvePool) external onlyOwner {

Because the external address is not verified against a registry (e.g. the curve registry 0x0000000022D53366457F9d5E68Ec105046FC4383), the validation could be sidestepped by a malicious owner.

Because of that setToAndFromCurve() could be replaced by just inputting the pool indexes, saving the STATICALL and SLOADS which, beside the extra small math, should save 2.1k * 2 (SLOAD) + 100 * 2 (STATICALL) = 4400 gas

Make Cowswap Fields Constants - 2.1k * 2 = 4200 gas

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L73-L74

        COW_SETTLEMENT = 0x9008D19f58AAbD9eD0D60971565AA8510560ab41;
        COW_RELAYER = 0xC92E8bdf79f0507f65a392b0ab4667716BFE0110;

These values are hardcoded in the initializer, it would be better to hardcode them as constants

Will save 2.1k each time you read them

Storage Read when function param is cheaper - 100 gas

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L82

The function is receiving a new _stakingContract but using the storage value to set approvals

Change

  IERC20Upgradeable(rewardToken).approve(
      stakingContract,
      type(uint256).max
  );

To

  IERC20Upgradeable(rewardToken).approve(
      _stakingContract,
      type(uint256).max
  );

Double SLOAD when MSTORE would be chaper (94 gas * 3) - 282 gas

• stakingToken
• stakingToken
• stakingToken

Double SLOAD when you already have cached value in memory - 97 gas

In both isntances you check with _isClaimAvailable but you already cached Claim memory info = warmUpInfo[_recipient]. To avoid an additional SLOAD (100 gas), you could just pass the info to the function

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L465-L467

    function claim(address _recipient) public {
        Claim memory info = warmUpInfo[_recipient];
        if (_isClaimAvailable(_recipient)) {

Also applies here https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L425-L430

        Claim storage info = warmUpInfo[_recipient];

        // if claim is available then auto claim tokens
        if (_isClaimAvailable(_recipient)) {
            claim(_recipient);
        }

Refactor to

_isClaimAvailable(info)

Double SLOAD - 94 gas

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L50-L59

    function canBatchContractByIndex(uint256 _index)
        external
        view
        returns (address, bool)
    {
        return (
            contracts[_index],
            IStaking(contracts[_index]).canBatchTransactions()
        );
    }

Would recommend refactoring to

address contract = contracts[_index];
  return (
      contract,
      IStaking(contract).canBatchTransactions()
  );

Which will save 94 gas

Expensive read from storage - 191 gas per iteration

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L14-L27

    function sendWithdrawalRequests() external {
        uint256 contractsLength = contracts.length;
        for (uint256 i; i < contractsLength; ) {
            if (
                contracts[i] != address(0) &&
                IStaking(contracts[i]).canBatchTransactions()
            ) {
                IStaking(contracts[i]).sendWithdrawalRequests();
            }
            unchecked {
                ++i;
            }
        }
    }

We could instead cache the value

address contract = contracts[i];

Saving 94 gas on every iteration which returns false And saving another 97 gas when doing the sendWithdrawalRequests

For a total of 191 gas per iteration because we use Memory intead of Storage

Same deal - reading contracts from storage instead of caching in memory - 94 gas per loop

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L29-L44

    function canBatchContracts() external view returns (Batch[] memory) {
        uint256 contractsLength = contracts.length;
        Batch[] memory batch = new Batch[](contractsLength);
        for (uint256 i; i < contractsLength; ) {
            bool canBatch = IStaking(contracts[i]).canBatchTransactions();
            batch[i] = Batch(contracts[i], canBatch);
            unchecked {
                ++i;
            }
        }
        return batch;
    }

We can save 94 gas per iteration here as well

Additional SLOAD when you already cached the value - 97 gas

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L319-L321

    function _requestWithdrawalFromTokemak(uint256 _amount) internal {
        ITokePool tokePoolContract = ITokePool(TOKE_POOL);
        uint256 balance = ITokePool(TOKE_POOL).balanceOf(address(this));

You can just use tokePoolContract at this point, saving 97 gas

Similarly you can use a memory cache here as well: https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L144-L147

        uint256 totalTokeAmount = IERC20Upgradeable(TOKE_TOKEN).balanceOf(
            address(this)
        );
        IERC20Upgradeable(TOKE_TOKEN).safeTransfer(

Unnecessary MSTORE - 6 gas * 2 = 12 gas

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L329-L345

    function _depositToTokemak(uint256 _amount) internal {
        ITokePool tokePoolContract = ITokePool(TOKE_POOL);
        tokePoolContract.deposit(_amount);
    }
    function _getTokemakBalance() internal view returns (uint256) {
        ITokePool tokePoolContract = ITokePool(TOKE_POOL);
        return tokePoolContract.balanceOf(address(this));
    }

You're storing the casted version of the address, but you could simply cast the storage value, avoiding the extra 6 gas

Change to:

    function _getTokemakBalance() internal view returns (uint256) {
        return ITokePool(TOKE_POOL).balanceOf(address(this));
    }

Additional instance: https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L120-L121

        ITokeReward tokeRewardContract = ITokeReward(TOKE_REWARD);

Avoid reading from storage if you have a memory copy - 97 * 11 = 1067 gas

All fields in initialize are being read from storage, each read costing an additional 97 gas over just reading from the memory parameters

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L77-L92

        if (CURVE_POOL != address(0)) {
            IERC20(TOKE_POOL).approve(CURVE_POOL, type(uint256).max);
            setToAndFromCurve();
        }

        IERC20(STAKING_TOKEN).approve(TOKE_POOL, type(uint256).max);
        IERC20Upgradeable(YIELDY_TOKEN).approve(
            LIQUIDITY_RESERVE,
            type(uint256).max
        );
        IERC20Upgradeable(YIELDY_TOKEN).approve(
            LIQUIDITY_RESERVE,
            type(uint256).max
        );
        IERC20Upgradeable(TOKE_TOKEN).approve(COW_RELAYER, type(uint256).max);

Can be changed to _curvePool != address(0) etc.. Each SLOAD is 100 gas vs the CALLDATALOAD which costs 3 gas

There's 11 ALL_CAPS storage vars, so the refactoring will save 1067 gas on the initializer call