Popcorn contest - nadin's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L134-L162 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L294-L301

Vulnerability details

Impact

Vault share price can be maliciously inflated on the initial deposit, leading to the next depositor losing assets due to precision issues.

Proof of Concept

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L134-L162

147:        shares = convertToShares(assets) - feeShares;

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L294-L301

If feeShares = 0 the first depositor of Vault can maliciously manipulate the share price by depositing the lowest possible amount (1 wei) of liquidity and then artificially inflating Vault.totalAssets.

This can inflate the base share price as high as 1:1e18 early on, which force all subsequence deposit to use this share price as a base and worst case, due to rounding down, if this malicious initial deposit front-run someone else depositing, this depositor will receive 0 shares and lost his deposited assets.

Tools Used

Manual Review

Recommended Mitigation Steps

This is a well-known issue, Uniswap and other protocols had similar issues when supply == 0. For the first deposit, mint a fixed amount of shares.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L294-L301 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L211-L240

Vulnerability details

Impact

Malicious users can withdraw the assets from the vault for free, effectively allowing them to drain the assets of the vault.

Proof of Concept

[H-01] Malicious Users Can Drain The Assets Of Vault The Vault.convertToShares function relies on the mulDiv Rounding Down function in Line 300 when calculating the number of shares needed in exchange for a certain number of assets. Note that the computation is rounded down, therefore, if the result is less than 1 (e.g. 0.9), Solidity will round them down to zero. Thus, it is possible that this function will return zero. File: Vault.sol https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L294-L301

    function convertToShares(uint256 assets) public view returns (uint256) {
    uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.


    return
        supply == 0
            ? assets
            : assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);
}

The Vault.withdraw function relies on the Vault.convertToShares function. In certain conditions, the Vault.convertToShares function in Line 300 will return zero if the withdrawal amount causes the division within the Vault.convertToShares function to round down to zero (usually due to a small amount of withdrawal amount).

If the Vault.convertToShares function in Line 300 returns zero, no shares will be burned at Line 233. Subsequently, in Line 233 , the contract will transfer the assets to the users. As a result, the users receive the assets without burning any of their shares, effectively allowing them to receive assets for free. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L233

233:        _burn(owner, shares);

Assume that the vault with the following state:

Total Asset = 1000 WETH Total Supply = 10 shares Assume that Alice wants to withdraw 99 WETH from the vault. Thus, she calls the Vault.withdraw(99 WETH) function. The Vault.convertToShares function will compute the number of shares that Alice needs to burn in exchange for 99 WETH. However, since Solidity rounds 0.99 down to 0, Alice does not need to burn a single share. She will receive 99 WETH for free.

assets.mulDiv(supply, totalAssets(), Math.Rounding.Down)
99 WETH.mulDiv(10 shares, 1000 WETH, Math.Rounding.Down)
    (99 * 10) / 1000
990 / 1000 = 0.99 = 0

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure that at least 1 share is burned when the users withdraw their assets. This can be mitigated by use the adapter.previewWithdraw function instead convertToShares function to round up instead of round down by when computing the number of shares to be burned. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L329-L337 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L343-L350

    function previewWithdraw(uint256 assets)
    public
    view
    virtual
    override
    returns (uint256)
{
    return _previewWithdraw(assets);
}


   

[N-01] USE OF BYTES.CONCAT() INSTEAD OF ABI.ENCODEPACKED(,)

Rather than using abi.encodePacked for appending bytes, since version 0.8.4, bytes.concat() is enabled Since version 0.8.4 for appending bytes, bytes.concat() can be used instead of abi.encodePacked(,). Context: 06 File: src/utils/MultiRewardEscrow.sol

104:    bytes32 id = keccak256(abi.encodePacked(token, account, amount, nonce));

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardEscrow.sol#L104

File: src/utils/MultiRewardStaking.sol

49:    _name = string(abi.encodePacked("Staked ", IERC20Metadata(address(_stakingToken)).name()));
50:    _symbol = string(abi.encodePacked("pst-", IERC20Metadata(address(_stakingToken)).symbol()));
461:          abi.encodePacked(

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L49-L50 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L461

File: src/vault/Vault.sol

94:            abi.encodePacked("Popcorn", name(), block.timestamp, "Vault")
680:                    abi.encodePacked(

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L94 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L680

File: src/vault/adapter/abstracts/AdapterBase.sol

648:                     abi.encodePacked(

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L648

[N-02] Using ecrecover is against best practice

Context: 03 Using ecrecover is against best practice. Preferably use ECDSA.recover instead. EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature unique. However it should be impossible to be a threat by now. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.4.0/contracts/cryptography/ECDSA.sol

File: src/utils/MultiRewardStaking.sol

459:      address recoveredAddress = ecrecover(

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L459-L479

File: src/vault/Vault.sol

678:            address recoveredAddress = ecrecover(

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L678

File: src/vault/adapter/abstracts/AdapterBase.sol

646:             address recoveredAddress = ecrecover(

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L646

[N-03] NON-LIBRARY/INTERFACE FILES SHOULD USE FIXED COMPILER VERSIONS, NOT FLOATING ONES

All contract

[NC-04] Return values of approve() not checked

Not all IERC20 implementations revert() when there's a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything File: src/utils/MultiRewardStaking.sol , src/vault/Vault.sol , src/vault/VaultController.sol https://github.com/code-423n4/2023-01-popcorn//blob/main/src/utils/MultiRewardStaking.sol https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/Vault.sol https://github.com/code-423n4/2023-01-popcorn//blob/main/src/vault/VaultController.sol

[N-05] NATSPEC COMMENTS SHOULD BE INCREASED IN CONTRACTS

Context: All Contracts

Description: It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability. https://docs.soliditylang.org/en/v0.8.15/natspec-format.html

Recommendation: NatSpec comments should be increased in contracts