Reserve Protocol - Invitational - auditor0517's results

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/erc20/RewardableERC20.sol#L86

Vulnerability details

Impact

Some rewards might be locked inside the contract due to the rounding loss.

Proof of Concept

_claimAndSyncRewards() claimed the rewards from the staking contract and tracks rewardsPerShare with the current supply.

    function _claimAndSyncRewards() internal virtual {
        uint256 _totalSupply = totalSupply();
        if (_totalSupply == 0) {
            return;
        }
        _claimAssetRewards();
        uint256 balanceAfterClaimingRewards = rewardToken.balanceOf(address(this));

        uint256 _rewardsPerShare = rewardsPerShare;
        uint256 _previousBalance = lastRewardBalance;

        if (balanceAfterClaimingRewards > _previousBalance) {
            uint256 delta = balanceAfterClaimingRewards - _previousBalance;
            // {qRewards/share} += {qRewards} * {qShare/share} / {qShare}
            _rewardsPerShare += (delta * one) / _totalSupply; //@audit possible rounding loss
        }
        lastRewardBalance = balanceAfterClaimingRewards;
        rewardsPerShare = _rewardsPerShare;
    }

It uses one as a multiplier and from this setting we know it has the same decimals as underlying(thus totalSupply).

My concern is _claimAndSyncRewards() is called for each deposit/transfer/withdraw in _beforeTokenTransfer() and it will make the rounding problem more serious.

1. Let's consider underlyingDecimals = 18. totalSupply = 10**6 with 18 decimals, rewardToken has 6 decimals. And total rewards for 1 year are 1M rewardToken for 1M totalSupply.
2. With the above settings, _claimAndSyncRewards() might be called every 1 min due to the frequent user actions.
3. Then expected rewards for 1 min are 1000000 / 365 / 24 / 60 = 1.9 rewardToken = 1900000 wei.
4. During the division, it will be 1900000 * 10**18 / (1000000 * 10**18) = 1. So users would lose almost 50% of rewards due to the rounding loss and these rewards will be locked inside the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

I think there would be 2 mitigation.

1. Use a bigger multiplier.
2. Keep the remainders and use them next time in _claimAndSyncRewards() like this.

    if (balanceAfterClaimingRewards > _previousBalance) {
        uint256 delta = balanceAfterClaimingRewards - _previousBalance; //new rewards
        uint256 deltaPerShare = (delta * one) / _totalSupply; //new delta per share

        // decrease balanceAfterClaimingRewards so remainders can be used next time
        balanceAfterClaimingRewards = _previousBalance + deltaPerShare * _totalSupply / one;

        _rewardsPerShare += deltaPerShare;
    }
    lastRewardBalance = balanceAfterClaimingRewards;

Assessed type

Math

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/stargate/StargateRewardableWrapper.sol#L48

Vulnerability details

Impact

The staked funds might be locked because the deposit/withdraw/transfer logic reverts.

Proof of Concept

In StargateRewardableWrapper, _claimAssetRewards() claims the accumulated rewards from the staking contract and it's called during every deposit/withdraw/transfer in _beforeTokenTransfer() and _claimAndSyncRewards().

    function _claimAssetRewards() internal override {
        stakingContract.deposit(poolId, 0);
    }

And in the stargate staking contract, deposit() calls updatePool() inside the function.

    function updatePool(uint256 _pid) public {
        PoolInfo storage pool = poolInfo[_pid];
        if (block.number <= pool.lastRewardBlock) {
            return;
        }
        uint256 lpSupply = pool.lpToken.balanceOf(address(this));
        if (lpSupply == 0) {
            pool.lastRewardBlock = block.number;
            return;
        }
        uint256 multiplier = getMultiplier(pool.lastRewardBlock, block.number);
        uint256 stargateReward = multiplier.mul(stargatePerBlock).mul(pool.allocPoint).div(totalAllocPoint); //@audit revert when totalAllocPoint = 0

        pool.accStargatePerShare = pool.accStargatePerShare.add(stargateReward.mul(1e12).div(lpSupply));
        pool.lastRewardBlock = block.number;
    }

    function deposit(uint256 _pid, uint256 _amount) public {
        PoolInfo storage pool = poolInfo[_pid];
        UserInfo storage user = userInfo[_pid][msg.sender];
        updatePool(_pid);
        ...
    }

The problem is updatePool() reverts when totalAllocPoint == 0 and this value can be changed by stargate admin using set().

So user funds might be locked like the below.

1. The stargate staking contract had one pool and totalAllocPoint = 10.
2. In StargateRewardableWrapper, some users staked their funds using deposit().
3. After that, that pool was removed by the stargate admin due to an unexpected reason. So the admin called set(0, 0) to reset the pool. Then totalAllocPoint = 0 now. In the stargate contract, it's not so critical because this contract has emergencyWithdraw() to rescue funds without caring about rewards. Normal users can withdraw their funds using this function.
4. But in StargateRewardableWrapper, there is no logic to be used under the emergency and deposit/withdraw won't work because _claimAssetRewards() reverts in updatePool() due to 0 division.

Tools Used

Manual Review

Recommended Mitigation Steps

We should implement a logic for an emergency in StargateRewardableWrapper.

During the emergency, _claimAssetRewards() should return 0 without interacting with the staking contract and we should use stakingContract.emergencyWithdraw() to rescue the funds.

Assessed type

Error

[L-01] Unsafe max approve

It's not recommended to approve type(uint256).max for safety. We should approve a relevant amount every time.

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/aave/StaticATokenLM.sol#L105

ASSET.safeApprove(address(pool), type(uint256).max);

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/stargate/StargateRewardableWrapper.sol#L39

pool_.approve(address(stakingContract_), type(uint256).max);

[L-02] Possible reentrancy

Users might manipulate wrapperPostPrinc inside the transfer hook. With the current underlyingComet token, there is no impact as it doesn't have any hook but it's recommended to add a nonReentrant modifier to _deposit()/_withdraw().

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol#L96

IERC20(address(underlyingComet)).safeTransferFrom(src, address(this), amount);

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol#L154

IERC20(address(underlyingComet)).safeTransfer(dst, (amount / 10) * 10);

[L-03] Unsafe downcasting

feeds[i].length might be downcasted wrongly when it's greater than type(uint8).max. maxFeedsLength() might pass the requirement when config.feeds has many elements(like 256).

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/curve/PoolTokens.sol#L330

maxLength = uint8(Math.max(maxLength, feeds[i].length));

[L-04] Reverts on 0 transfer

It deposits 0 amount to the staking contract to claim rewards but it might revert during the 0 transfer. There is no problem with the current lpToken but good to keep in mind.

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/stargate/StargateRewardableWrapper.sol#L48

stakingContract.deposit(poolId, 0);

[L-05] Wrong comment

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/Asset.sol#L146

// oracleTimeout <= delta <= oracleTimeout + priceTimeout ==========> oracleTimeout < delta < oracleTimeout + priceTimeout

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/compoundv3/WrappedERC20.sol#L298

     * - when `from` is zero, `amount` tokens will be minted for `to`. //@audit should remove, no hook if `from` or `to` is zero
     * - when `to` is zero, `amount` of ``from``'s tokens will be burned.

[L-06] Unsafe permission

WrappedERC20 uses a permission mechanism and operators can have an infinite allowance once approved. It might be inconvenient/dangerous for some users.

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol#L84

    function _deposit(
        address operator,
        address src,
        address dst,
        uint256 amount
    ) internal {
        if (!hasPermission(src, operator)) revert Unauthorized(); //@audit infinite allowance
        ...
    }

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol#L140

    function _withdraw(
        address operator,
        address src,
        address dst,
        uint256 amount
    ) internal {
        if (!hasPermission(src, operator)) revert Unauthorized(); //@audit infinite allowance
        ...
    }

[L-07] Typical first depositor issue in RewardableERC4626Vault

It's recommended to follow the instructions.

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/erc20/RewardableERC4626Vault.sol#L20

abstract contract RewardableERC4626Vault is ERC4626, RewardableERC20 {}

[L-08] Needless accrue for src

We don't need to accrue for src because we don't use any information of src and that info will be accrued during the underlyingComet transfer.

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol#L91

    underlyingComet.accrueAccount(address(this));
    underlyingComet.accrueAccount(src); //@audit needless accrue

    CometInterface.UserBasic memory wrappedBasic = underlyingComet.userBasic(address(this));
    int104 wrapperPrePrinc = wrappedBasic.principal;

    IERC20(address(underlyingComet)).safeTransferFrom(src, address(this), amount); 

[N-01] Typo

_accumuatedRewards => _accumulatedRewards

• https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/erc20/RewardableERC20.sol#L57

uint256 _accumuatedRewards = accumulatedRewards[account];