Popcorn contest - 0xWeiss's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol?plain=1#L153

Vulnerability details

Summary

When an erc777 is used as asset for the vault, you can re-enter the _mint function by minting the double you would have minted with a normal erc20 token.

Vulnerability Detail

The following 2 functions allow minting in the vault. One is for depositing a certain amount of assets, in this case, erc777 and getting shares in exchange, and the second one is to calculate the number of assets you have to send to mint "x" shares. The problem relies on the following lines:

deposit function:

   _mint(receiver, shares);

    asset.safeTransferFrom(msg.sender, address(this), assets);

    adapter.deposit(assets, address(this));

The erc777 has a callback to the owner of the tokens before doing transferFrom. In this case, it is a vulnerable function because it is minting the shares before we transfer the tokens. So, on the callback that transferFrom makes to our attacker contract, we directly call the other mint function that is also publicly callable by anyone. The reason why we can't re-enter the deposit is that it has a nonReentrant modifier, so we have to perform a cross-function re-entrancy.

mint function:

    _mint(receiver, shares);

    asset.safeTransferFrom(msg.sender, address(this), assets);

    adapter.deposit(assets, address(this));

So eventually, you will be able to get twice as much shares everytime you deposit.

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol?plain=1#L134-L157 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol?plain=1#L170-L198

Impact

Eventually you will be able to mint twice as much as you provide assets.

Tool used

Manual Review

Recommendation

The changes that have to be made are 2:

Either state clearly that erc777 are not supported, or change the flow of the function, transferring first the assets and then getting the shares.

   asset.safeTransferFrom(msg.sender, address(this), assets); //changed to the top of the function
    _mint(receiver, shares);
    adapter.deposit(assets, address(this));

[L-01] CHANGE VARIBALES TO IMMUTABLE

The following variables, do not have a function to update them and are initialized in the constructor, therefore they should be immutable or constant and hardcode the addresses.

IBeefyVault public beefyVault;
IBeefyBooster public beefyBooster;
IBeefyBalanceCheck public beefyBalanceCheck;

The variables are here: https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/beefy/BeefyAdapter.sol#L27-L29

[L-02] UNUSAGE OF ECDSA LIBRARY AND WRONGLY CHECKED THE RECOVERED ADDRESS

The issue relies on the permit function. Where Popcorn is using the recover opcode instead of the ECDSA library from openzeppelin. That does not bring any issues normally, but it is the best practice. Also, due to not using the ECDSA library,
the function requirement can be bypassed

function permit(
    address owner,
    address spender,
    uint256 value,
    uint256 deadline,
    uint8 v,
    bytes32 r,
    bytes32 s
) public virtual {
    if (deadline < block.timestamp) revert PermitDeadlineExpired(deadline);

    // Unchecked because the only math done is incrementing
    // the owner's nonce which cannot realistically overflow.
    unchecked {
        address recoveredAddress = ecrecover(
            keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    DOMAIN_SEPARATOR(),
                    keccak256(
                        abi.encode(
                            keccak256(
                                "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"
                            ),
                            owner,
                            spender,
                            value,
                            nonces[owner]++,
                            deadline
                        )
                    )
                )
            ),
            v,
            r,
            s
        );
        if (recoveredAddress == address(0) || recoveredAddress != owner)
            revert InvalidSigner(recoveredAddress);

        _approve(recoveredAddress, spender, value);
    }

As you can notice, the if statement:

   if (recoveredAddress == address(0) || recoveredAddress != owner)

it is not truly well implemented, because in the function it is not checked that owner != address(0). So, if you pass the owner as address(0) and random signature values. The recovered address will be 0. So it will bypass the check

    (recoveredAddress == address(0) || recoveredAddress != owner)

because it has and OR || not and &&.

Recommendation

Change the way in how you formulate that if statement:

     if (recoveredAddress != address(0) && recoveredAddress == owner)

and replace recover for the ECDSA library by OZ