Aura admins can lock funds forever

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraLocker.sol#L195-L198
https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/BaseRewardPool.sol#L125-L131

Vulnerability details

Impact

Aura admins can lock funds forever, essentially rugging their users

Proof of Concept

By adding a lot of reward tokens:

File: contracts/AuraLocker.sol   #1

195       function addReward(address _rewardsToken, address _distributor) external onlyOwner {
196           require(rewardData[_rewardsToken].lastUpdateTime == 0, "Reward already exists");
197           require(_rewardsToken != address(stakingToken), "Cannot add StakingToken as reward");
198           rewardTokens.push(_rewardsToken);

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraLocker.sol#L195-L198

And then transferring admin privs to another account and then throwing away that account's private key (so that recovery can't be started by calling shutdown() alter), an admin can cause user operations to revert whenever they attempt to get their funds:

File: contracts/AuraLocker.sol   #2

170       modifier updateReward(address _account) {
171           {
172               Balances storage userBalance = balances[_account];
173               uint256 rewardTokensLength = rewardTokens.length;
174               for (uint256 i = 0; i < rewardTokensLength; i++) {
175                   address token = rewardTokens[i];
176                   uint256 newRewardPerToken = _rewardPerToken(token);
177                   rewardData[token].rewardPerTokenStored = newRewardPerToken.to96();
178                   rewardData[token].lastUpdateTime = _lastTimeRewardApplicable(rewardData[token].periodFinish).to32();
179                   if (_account != address(0)) {
180                       userData[_account][token] = UserData({
181                           rewardPerTokenPaid: newRewardPerToken.to128(),
182                           rewards: _earned(_account, token, userBalance.locked).to128()
183                       });
184                   }
185               }
186           }
187           _;
188       }

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraLocker.sol#L170-L188

The modifier is used on this function, and all functions where the user can claim funds:

File: contracts/AuraLocker.sol   #3

370       // Withdraw all currently locked tokens where the unlock time has passed
371       function _processExpiredLocks(
372           address _account,
373           bool _relock,
374           address _rewardAddress,
375           uint256 _checkDelay
376       ) internal updateReward(_account) {

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraLocker.sol#L370-L376

I confirmed with the sponsor that the admin being able to steal user funds counts for the 100k bonus, and the above fulfills that requirement

The BaseRewardsPool has a similar issue:

File: convex-platform/contracts/contracts/BaseRewardPool.sol   #4

125       function addExtraReward(address _reward) external returns(bool){
126           require(msg.sender == rewardManager, "!authorized");
127           require(_reward != address(0),"!reward setting");
128   
129           extraRewards.push(_reward);
130           return true;
131       }

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/BaseRewardPool.sol#L125-L131

File: convex-platform/contracts/contracts/BaseRewardPool.sol   #5

222       function withdraw(uint256 amount, bool claim)
223           public
224           updateReward(msg.sender)
225           returns(bool)
226       {
227           require(amount > 0, 'RewardPool : Cannot withdraw 0');
228   
229           //also withdraw from linked rewards
230           for(uint i=0; i < extraRewards.length; i++){
231               IRewards(extraRewards[i]).withdraw(msg.sender, amount);
232           }

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/BaseRewardPool.sol#L222-L232

Tools Used

Code inspection

Recommended Mitigation Steps

Allow the caller to provide an offset and a length, to allow the batch-claiming of rewards

[0xMaharishi]

This is severe griefing by the governance multisig and is not valid because there is no way for them to steal the funds and nothing to gain here. That said, it would make sense to add a max cap on the amount of reward contracts that could be added

[dmvt]

I'm also dropping this to a QA issue. As with the other one, it's valid, but extremely unlikely to happen. Governance has literally no reason to do this. It costs money and breaks their own protocol without providing any returns.

This one is also not a critical bug and also not eligible for the $100k bonus.

[dmvt]

Grouping this with the warden’s QA report, #173