Popcorn contest - Dewaxindo's results

Lack of Input Validation

For defense-in-depth purposes, it is recommended to perform additional validation against the amount the user is attempting to deposit, mint, withdraw and redeem to ensure that the submitted amount is valid.

OpenZeppelinTokenizedVault.sol#L95

1. Function deposit()

Vault.sol#L134-L158

 function deposit(uint256 assets, address receiver)
        public
        nonReentrant
        whenNotPaused
        syncFeeCheckpoint
        returns (uint256 shares)
    {
       require(assets <= maxDeposit(receiver), "deposit more than max");

2. Function mint()

Vault.sol#L170-L198

 function mint(uint256 shares, address receiver)
        public
        nonReentrant
        whenNotPaused
        syncFeeCheckpoint
        returns (uint256 assets)
    {
        require(shares <= maxMint(receiver), "mint more than max");

3. Function withdraw()

Vault.sol#L211-L240

 function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public nonReentrant syncFeeCheckpoint returns (uint256 shares) {
        require(assets <= maxWithdraw(owner), "withdraw more than max");

4. Function redeem()

Vault.sol#L253-L278

function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public nonReentrant returns (uint256 assets) {
        require(shares <= maxRedeem(owner), "redeem more than max");

5. Function deposit()

AdapterBase.sol#L110-L122

 function deposit(uint256 assets, address receiver)
        public
        virtual
        override
        returns (uint256)
    {
       require(assets <= maxDeposit(receiver), "deposit more than max");

6. Function mint()

AdapterBase.sol#L129-L141

 function mint(uint256 shares, address receiver)
        public
        virtual
        override
        returns (uint256)
    {
        require(shares <= maxMint(receiver), "mint more than max");

7. Function withdraw()

AdapterBase.sol#L173-L185

function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public virtual override returns (uint256) {
        require(assets <= maxWithdraw(owner), "withdraw more than max");

8. Function redeem()

AdapterBase.sol#L193-L204

function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public virtual override returns (uint256) {
        require(shares <= maxRedeem(owner), "redeem more than max");

<br>

Critical Address Changes Should Use Two-Step Procedure

Context

Vault.sol#L553-L559

function setFeeRecipient(address _feeRecipient) external onlyOwner {
        if (_feeRecipient == address(0)) revert InvalidFeeRecipient();

        emit FeeRecipientUpdated(feeRecipient, _feeRecipient);

        feeRecipient = _feeRecipient;
    }

Recommendation

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

<br>

USE OWNABLE2STEPUPGRADEABLE INSTEAD OF OWNABLEUPGRADEABLE CONTRACT

Context

• Vault.sol#L13
• MultiRewardStaking.sol#L10
• AdapterBase.sol#L15

Recommendation

acceptOwnership function is used to accept Ownership from OwnableUpgradeable.sol.

There is another Openzeppelin Ownable contract (Ownable2StepUpgradeable.sol) has acceptOwnership function , use it is more secure due to 2-stage ownership transfer.

Ownable2StepUpgradeable.sol

Pleaser refer to this Gist link:

https://gist.github.com/BadGenius22/d39f00efe31c1cacb1f159182f4c0cce