Popcorn contest - jasonxiale's results

Lines of code

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

Vulnerability details

Impact

accruedManagementFee calculate managementFee incorrectly

Proof of Concept

Supposed during initialize, fees.management is 0, and feesUpdatedAt is set to block.timestamp. Then after 10 years, the owner calls proposeFees to set management > 0, and then Vault.changeFees is called after Vault.proposedFeeTime + Vault.quitPeriod.

When called Vault.accruedManagementFee to calculate management fee,

function accruedManagementFee() public view returns (uint256) {
    uint256 managementFee = fees.management;
    return
        managementFee > 0
            ? managementFee.mulDiv(
                totalAssets() * (block.timestamp - feesUpdatedAt),
                SECONDS_PER_YEAR,
                Math.Rounding.Down
            ) / 1e18
            : 0;
}

in the code above, block.timestamp - feesUpdatedAt == 10 years, which I don't think it's correct. And I think feesUpdatedAt should be the block.timestamp when Vault.changeFees is called

Tools Used

Foundry

Recommended Mitigation Steps

update Vault.feesUpdatedAt properly

Lines of code

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

Vulnerability details

Impact

When calling MultiRewardStaking.addRewardToken, there is no limit how many tokens can be created, so it's possible that MultiRewardStaking. rewardTokens's length is large than 256. In such case, uint8 in MultiRewardStaking.accrueRewards will be overflow.

modifier accrueRewards(address _caller, address _receiver) {
    IERC20[] memory _rewardTokens = rewardTokens;
    for (uint8 i; i < _rewardTokens.length; i++) {
        IERC20 rewardToken = _rewardTokens[i];
        RewardInfo memory rewards = rewardInfos[rewardToken];

        if (rewards.rewardsPerSecond > 0) _accrueRewards(rewardToken, _accrueStatic(rewards));
        _accrueUser(_receiver, rewardToken);

        // If a deposit/withdraw operation gets called for another user we should accrue for both of them to avoid potential issues like in the Convex-Vulnerability
        if (_receiver != _caller) _accrueUser(_caller, rewardToken);
    }
    _;
}

Same issue might happens in MultiRewardStaking.claimRewards

Proof of Concept

put the code below into MultiRewardStaking.t.sol, and run

function testMultipleRewardsToken() public {
    MockERC20[257]    memory     rewardTokenLarge;
    IERC20[257]         memory     iRewardTokenLarge;

    for(uint256 i; i < 257; i++) {
            rewardTokenLarge[i]    = new MockERC20("RewardsTokenLarge", "RTKNL", 18); 
            iRewardTokenLarge[i] = IERC20(address(rewardTokenLarge[i]));
    }        

    for(uint256 i; i < 257; i++) {
            _addRewardToken(rewardTokenLarge[i]);
    }        

    stakingToken.mint(ada, 5 ether);

    vm.startPrank(ada);
    stakingToken.approve(address(staking), 5 ether);
    staking.deposit(1 ether);
}

Tools Used

Foundry

Recommended Mitigation Steps

diff --git a/src/utils/MultiRewardStaking.sol b/src/utils/MultiRewardStaking.sol
index 95ebefd..253d12f 100644
--- a/src/utils/MultiRewardStaking.sol
+++ b/src/utils/MultiRewardStaking.sol
@@ -168,7 +168,7 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable {
    * @dev A percentage of each reward can be locked in an escrow contract if this was previously configured.
    */
   function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
-    for (uint8 i; i < _rewardTokens.length; i++) {
+    for (uint i; i < _rewardTokens.length; i++) {
       uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];
 
       if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);
@@ -370,7 +370,7 @@ contract MultiRewardStaking is ERC4626Upgradeable, OwnedUpgradeable {
   /// @notice Accrue rewards for up to 2 users for all available reward tokens.
   modifier accrueRewards(address _caller, address _receiver) {
     IERC20[] memory _rewardTokens = rewardTokens;
-    for (uint8 i; i < _rewardTokens.length; i++) {
+    for (uint i; i < _rewardTokens.length; i++) {
       IERC20 rewardToken = _rewardTokens[i];
       RewardInfo memory rewards = rewardInfos[rewardToken];

Lines of code

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

Vulnerability details

Impact

Vault.fees can be overwritten by Vault.changeFees, which will affect the whole reward logic

Proof of Concept

in initialize, fee is set to fees_, if fees_ is not empty(say VaultFees({ deposit: 1e17, withdrawal: 2e17, management: 3e17, performance: 4e17 })), after quitPeriod, any user can calll function changeFees, in such case, proposedFees is empty, so fees will be overwritten to empty The same issue in (Vault.sol#L578)[https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L578], proposedAdapter can be address(0)

function setUp() public {
    vm.label(feeRecipient, "feeRecipient");
    vm.label(alice, "alice");
    
    asset = new MockERC20("Mock Token", "TKN", 18);
    adapter = new MockERC4626(IERC20(address(asset)), "Mock Token Vault", "vwTKN");
    
    address vaultAddress = address(new Vault());
    vm.label(vaultAddress, "vault");
    
    vault = Vault(vaultAddress);
    vault.initialize(
        IERC20(address(asset)),
        IERC4626(address(adapter)),
        VaultFees({ deposit: 1e17, withdrawal: 2e17, management: 3e17, performance: 4e17 }),
        feeRecipient,
        address(this)
    );
} 

function testChangeFeesNonOwner() public {
    {     
    (uint256 deposit, uint256 withdrawal, uint256 management, uint256 performance)
            = vault.fees();
    
    emit log_named_uint("deposit        :        ", deposit);
    emit log_named_uint("withdrawal :        ", withdrawal);
    emit log_named_uint("management :        ", management);
    emit log_named_uint("performance:        ", performance);
    }
    vm.warp(block.timestamp + 3 days);
    
    vm.prank(alice);
    vault.changeFees();

    {
    (uint256 deposit, uint256 withdrawal, uint256 management, uint256 performance)
            = vault.fees();

    emit log_named_uint("deposit        :        ", deposit);
    emit log_named_uint("withdrawal :        ", withdrawal);
    emit log_named_uint("management :        ", management);
    emit log_named_uint("performance:        ", performance);
    }
}

Tools Used

foundry

Recommended Mitigation Steps

add some check