Alchemix contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/adapters/yearn/YearnTokenAdapter.sol#L13 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/adapters/yearn/YearnTokenAdapter.sol#L43

Vulnerability details

Impact

YearnTokenAdapter allows slippage of 100% when withdrawing from the vault which will cause a loss of funds.

Here's the documentation straight from the vault contract: https://github.com/yearn/yearn-vaults/blob/main/contracts/Vault.vy#L1025-L1073 It allows the user to specify the maxLoss as the last parameter. It determines how many shares can be burned to fulfill the withdrawal. Currently, the contract uses 10.000 which is 100%. Meaning there is no slippage check at all. This is bound to cause a loss of funds.

I'd suggest letting the user determine the slippage check themselves instead of hardcoding it.

Proof of Concept

Current maxLoss value: https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/adapters/yearn/YearnTokenAdapter.sol#L13

call to Yearn vault's withdraw() function: https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/adapters/yearn/YearnTokenAdapter.sol#L43

Tools Used

none

Recommended Mitigation Steps

Allow the user to specify the slippage value themselves:

    function unwrap(uint256 amount, address recipient, uint maxLoss) external override returns (uint256) {
        TokenUtils.safeTransferFrom(token, msg.sender, address(this), amount);

        uint256 balanceBefore = TokenUtils.safeBalanceOf(token, address(this));

        uint256 amountWithdrawn = IYearnVaultV2(token).withdraw(amount, recipient, maxLoss);

        uint256 balanceAfter = TokenUtils.safeBalanceOf(token, address(this));

        // If the Yearn vault did not burn all of the shares then revert. This is critical in mathematical operations
        // performed by the system because the system always expects that all of the tokens were unwrapped. In Yearn,
        // this sometimes does not happen in cases where strategies cannot withdraw all of the requested tokens (an
        // example strategy where this can occur is with Compound and AAVE where funds may not be accessible because
        // they were lent out).
        if (balanceBefore - balanceAfter != amount) {
            revert IllegalState();
        }

        return amountWithdrawn;
    }

If you don't want to change the ITokenAdapter interface you can also leave the value blank. The vault will then use the default value (0.01%)

Report

• Low
  • L-01: AlchemicTokenV2.mint() doesn't follow specs
  • L-02: AlchemicTokenv2.setFlashFee() should be constrained to allow only reasonable values
  • L-03: Use two-step process for critical address changes consistently
  • L-04: StakingPools's governance address transfer doesn't follow standard procedure
  • L-05: TransmterConduit doesn't follow specs
  • L-06: WETHGateway's whitelist state variable can't be updated
• Non-Critical
  • N-01: AlchemicTokenV2.setWhitelist() & setMaxFlashLoan() should emit an event
  • N-02: gALCX.transferOwnership() should emit an event

Low

L-01: AlchemicTokenV2.mint() doesn't follow specs

In the function's comment you state that the function should revert if msg.sender has exceeded their mintable ceiling. But, there's no mintable ceiling in the contract. So either the mint() function is missing a relevant contraint or the comment is not up-to-date.

The AlchemicTokenV2Base and AlchemicTokenV1 contracts have it tho.

Relevant code:

• https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L97-L111
• https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2Base.sol#L117

L-02: AlchemicTokenv2.setFlashFee() should be constrained to allow only reasonable values

Currently, the admin can set the fee to whatever amount they want. I think it would be reasonable to add constraints her to limit it the possibilities. Most people tend to limit fees to < 100% although you could go constrain it even more if you don't expect to go over a certain number.

Relevant code:

• https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L93

L-03: Use two-step process for critical address changes consistently

In some of your contracts you already use a two-step process to change the admin/governance address. But, you don't do it consistently. Multiple contracts just use OpenZeppelin's Ownable contract which has a direct transfer. I'd encourage you to use the two-step process throughout all the contracts since it's safer.

Relevant code:

• https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/gALCX.sol#L40

L-04: StakingPools's governance address transfer doesn't follow standard procedure

The two-step process doesn't allow the zero address being set as the pending address. But, that should be possible. It allows the current governance address to reset the pending address. Otherwise, to revoke the pending address you have to set it to some other value.

Also, when the pending address accepts, it the function should reset the pending address back to the zero address.

So it would look like this:

  function setPendingGovernance(address _pendingGovernance) external onlyGovernance {
    pendingGovernance = _pendingGovernance;

    emit PendingGovernanceUpdated(_pendingGovernance);
  }

  function acceptGovernance() external {
    require(msg.sender == pendingGovernance, "StakingPools: only pending governance");
    address _pendingGovernance = pendingGovernance;
    governance = _pendingGovernance;
    pendingGovernance = address(0);

    emit GovernanceUpdated(_pendingGovernance);
  }

Relevant code:

• https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/StakingPools.sol#L123-L137

L-05: TransmterConduit doesn't follow specs

The TransmuterConduit contract has a state variable sourceTransmuter from which the funds should be migrated (according to comment). But, the distribute() function pulls the funds from the origin parameter instead.

Relevant code:

• https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterConduit.sol#L15
• https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/TransmuterConduit.sol#L34-L37

L-06: WETHGateway's whitelist state variable can't be updated

The contract has two state variables: WETH & whitelist. WETH is already immutable but whitelist isn't. But, there's no way to update the value of whitelist.

Incase you want to update the variable, add the necessary function. Otherwise, set the variable to be immutable to save gas.

Relevant code:

• https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/WETHGateway.sol#L20

Non-Critical

N-01: AlchemicTokenV2.setWhitelist() & setMaxFlashLoan() should emit an event

Since the functions makes changes to critical parts of the contract's configuration you should emit an event there.

Relevant code:

• https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L120
• https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemicTokenV2.sol#L165

N-02: gALCX.transferOwnership() should emit an event

Since the functions makes changes to critical parts of the contract's configuration you should emit an event there.

Relevant code:

• https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/gALCX.sol#L40