Yeti Finance contest - Ruhum's results

Handle

Ruhum

Vulnerability details

Impact

The contract's setAddresses() function used to initialize it can be called by anybody. It makes it susceptible to being front-run. Somebody else initializes the contract without the original deployer being able to change it since it can only be initialized once. There should be access controls so only the owner is able to initialize it.

Proof of Concept

https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol#L102

Tools Used

none

Recommended Mitigation Steps

add access controls

Handle

Ruhum

Vulnerability details

Impact

Assigning a new address to be in control of a contract should always be done in a two-step process. Otherwise, you might potentially lose access by assigning it to an address for which you don't have the key. By using a two-step process you make sure that everything works as expected.

Using the current owner you assign a new pending owner. Using the pending owner you then claim ownership. The current owner is then overwritten by the pending one and the pending owner address is cleared.

Proof of Concept

https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/YetiFinanceTreasury.sol#L28

Tools Used

none

Recommended Mitigation Steps

Check out the Compound Timelock contract as an example: https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol#L45-L58

Handle

Ruhum

Vulnerability details

Impact

All the other passed variables are checked. Only _whitelistAddress is ignored. This allows passing a zero function which would break the functionality.

Proof of Concept

https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/CollSurplusPool.sol#L51-L54

Tools Used

none

Recommended Mitigation Steps

add checkContract(_whitelistAddress)

Handle

Ruhum

Vulnerability details

Impact

Structs should always be packed to save gas

Proof of Concept

• https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/PriceFeed.sol#L56

• https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/PriceFeed.sol#L64

• https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/Dependencies/Whitelist.sol#L28

Tools Used

none

Recommended Mitigation Steps

Pack structs, e.g.:

// unpacked, higher gas (6 storage slots):
    struct CollateralParams {
        uint256 ratio;
        address oracle;
        uint256 decimals;
        bool active;
        address priceCurve;
        uint256 index;
        bool isWrapped;
    }

// packed (needs 5 storage slots)
    struct CollateralParams {
        uint256 ratio;
        uint256 decimals;
        uint256 index;
        address oracle;
        bool active;
        address priceCurve;
        bool isWrapped;
    }

Handle

Ruhum

Vulnerability details

Impact

View functions consume less gas. WJLP.getPendingRewards() is technically also a view function but not specified as one. Because the IMasterChefJoeV2 interface used by the contract is wrong. It says poolInfo() is not a view function, which it is.

Proof of Concept

getPendingRewards: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol#L190

Faulty interface function: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/AssetWrappers/WJLP/WJLP.sol#L29

actually poolinfo is just an array so its getter is a view function: https://github.com/traderjoe-xyz/joe-core/blob/main/contracts/MasterChefJoeV2.sol#L85

Tools Used

none

Recommended Mitigation Step

Declare both functions as view to save gas

Handle

Ruhum

Vulnerability details

Impact

The Ownable contract used in the codebase doesn't allow transferring the ownership to a different address.

Most contracts that make use of Ownable always guard only a single function which after being called renounces the ownership. In that case, it should be fine since those are functions used to configure the contracts which make them usable, to begin with.

But, there are two contracts that don't renounce their ownership. Since those contracts want to keep ownership, it might come in handy in the future to be able to transfer the ownership to another address. As it is, the owner key is a single point of failure since there's no way to assign access to a different address.

Proof of Concept

Contracts that don't renounce ownership:

• Whitelist
• ThreePieceWiseLinearPriceCurve

Ownable contract used throughout the codebase: https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/Dependencies/Ownable.sol

Tools Used

none

Recommended Mitigation Steps

Either add the transfer functionality to the base contract or to the two contracts which don't want to renounce their ownership.

Also, consider using a two-step process to be safe. Here's an example from Compound's Timelock contract