Omniscia Echidna Finance Audit

VeEcdRewardsPool Manual Review Findings

VeEcdRewardsPool Manual Review Findings

VER-01M: Arbitrary Account Staking

Description:

The getReward function can be invoked by anyone and utilizes an input _account argument. While the funds will always be rewarded to the said _account, it is possible a user may not want to stake their reward but another malicious user stakes them on their behalf.

Example:

contracts/rewards/VeEcdRewardsPool.sol
124function getReward(address _account, bool _stake)
125    public
126    _updateReward(_account)
127    returns (bool)
128{
129    uint256 reward = rewards[_account];
130    if (reward > 0) {
131        rewards[_account] = 0;
132

133        SafeERC20.safeApprove(rewardToken, ptpDepositor, 0);
134        SafeERC20.safeApprove(rewardToken, ptpDepositor, reward);
135

136        IPTPDepositor(ptpDepositor).deposit(reward, false);
137

138        uint256 ecdCrvBalance = ecdPtpToken.balanceOf(address(this));
139        if (_stake) {
140            SafeERC20.safeApprove(ecdPtpToken, ecdPtpRewardPool, 0);
141            SafeERC20.safeApprove(
142                ecdPtpToken,
143                ecdPtpRewardPool,
144                ecdCrvBalance
145            );
146

147            IEcdPtpStaking(ecdPtpRewardPool).depositFor(
148                _account,
149                ecdCrvBalance
150            );
151        } else {
152            ecdPtpToken.safeTransfer(_account, ecdCrvBalance);
153        }
154        emit RewardPaid(_account, ecdCrvBalance);
155    }
156    return true;
157}

Recommendation:

We advise the function to be invoke-able only by the members themselves and the _account argument to be omitted from it.

Alleviation:

The code was simplified to only claim the reward and disallow immediate staking thereby nullifying this exhibit.

VER-02M: Deprecated Approval Paradigm

Description:

The linked statements utilize the safeApprove function that has been officially deprecated.

Example:

contracts/rewards/VeEcdRewardsPool.sol
130if (reward > 0) {
131    rewards[_account] = 0;
132

133    SafeERC20.safeApprove(rewardToken, ptpDepositor, 0);
134    SafeERC20.safeApprove(rewardToken, ptpDepositor, reward);
135

136    IPTPDepositor(ptpDepositor).deposit(reward, false);
137

138    uint256 ecdCrvBalance = ecdPtpToken.balanceOf(address(this));
139    if (_stake) {
140        SafeERC20.safeApprove(ecdPtpToken, ecdPtpRewardPool, 0);
141        SafeERC20.safeApprove(
142            ecdPtpToken,
143            ecdPtpRewardPool,
144            ecdCrvBalance
145        );
146

147        IEcdPtpStaking(ecdPtpRewardPool).depositFor(
148            _account,
149            ecdCrvBalance
150        );
151    } else {
152        ecdPtpToken.safeTransfer(_account, ecdCrvBalance);
153    }
154    emit RewardPaid(_account, ecdCrvBalance);
155}

Recommendation:

We advise a direct approve instruction to be utilized instead as the statements are indeed meant to replace any previously set allowance to the new one.

Alleviation:

A direct approve statement is now utilized in all referenced instances in place of the deprecated paradigm.

Prev

RewardPool.sol (RPL-M)

Next

VirtualBalanceRewardPool.sol (VBR-M)

On this page