Anchor contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/distributor/src/contract.rs#L140 https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/community/src/contract.rs#L69

Vulnerability details

Impact

It seems that the owner is only allowed to spend amount uptil config.spend_limit. However it was observed that this config.spend_limit is never decreased even if owner has spend an amount. This makes config.spend_limit useless as owner can simply send 2-multiple transactions each of config.spend_limit which will all pass and hence bypassing the spend limit placed on owner

Proof of Concept

1. Assume spend limit of 100 is placed on owner
2. Owner simply calls the spend function at either distributor or community contract with amount 100
3. Ideally after this transaction owner should not be allowed to perform any more spend operation
4. since config.spend_limit remains unchanged, owner can call step 2 multiple times which will spend amount 100 several times bypassing spend limit

Recommended Mitigation Steps

After successful spend, the config.spend_limit should be decreased by the amount spend

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-bAsset-contracts/contracts/anchor_basset_reward/src/user.rs

Vulnerability details

It was observed that execute_claim_rewards/execute_decrease_balance/execute_increase_balance are missing to update the global index before calculating user rewards in anchor_basset_reward contract This can lead to serious consequences:

1. execute_increase_balance function: Since global index is not updated, newly added balance will onboard with previous global index which could be lesser than current global index. If global index is updated later and user now claims, user will get higher reward for the amount he added which is incorrect
2. execute_claim_rewards/execute_decrease_balance: Since global index is not updated, user claim amount will be calculated based on older global index which means user will receive less payout than he was entitled to

Contract

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-bAsset-contracts/contracts/anchor_basset_reward/src/user.rs

Proof of Concept

1. Global index is 1
2. Multiple transaction happens where users have claimed and new balances have been added and decreased
3. Since execute_update_global_index function was not called so global index remains 1 instead of 1+a
4. Assume User A has existing balance of 1000.
5. execute_increase_balance function is called for User A with a balance amount of 500.
6. Since global index is still 1, reward for user A is calculated on amount 1000 with global index 1. Also amount 500 is added for user A and holder.index is set to 1 instead of 1+x
7. Admin calls execute_update_global_index function which update global index to 1+x
8. User A call execute_claim_rewards which computes the rewards for full amount 1000+500 with global index 1+x. This is wrong since global index was not updated and should have already been 1+x when user added 500 amount

Recommendation:

This should be revised as done in below contract: https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-bEth-contracts/contracts/anchor_beth_reward/src/user.rs

anchor_basset_hub contract :: execute_register_validator function

Function: execute_register_validator

Contract: https://github.com/Anchor-Protocol/anchor-bAsset-contracts/blob/master/contracts/anchor_basset_hub/src/config.rs#L114

Problem: If a Validator is already registered there is no need of further processing. Check for same is missing

Recommendation: Add a check to verify if the given validator is already whitelisted in which case directly return. Use is_valid_validator in state.rs for this purpose

gov contract :: withdraw_voting_tokens function

Function: withdraw_voting_tokens

Contract: https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/gov/src/staking.rs#L87

Problem: Gas is wasted if withdraw_share is computed as 0

Recommendation: Add a check for withdraw_share>0, otherwise return

gov contract :: create_poll function

Function: create_poll

Contract: https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/gov/src/contract.rs#L281

Recommendation: Change state.poll_count += 1; to state.poll_count = poll_id; to perform gas saving

gov contract :: cast_vote

Function: cast_vote

Contract: https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/gov/src/contract.rs#L582

Problem: If amount is 0 then user vote gets wasted and also cause gas wastage

Recommendation: check amount!=0

community contract :: execute function

Function: execute

Contract: https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/community/src/contract.rs#L35

Recommendation: Since both function require governance, governance check can be placed in execute instead of placing individually in UpdateConfig and Spend as done in https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-token-contracts/contracts/vesting/src/contract.rs

anchor_basset_reward :: execute_increase_balance/execute_decrease_balance

Function: execute_increase_balance/execute_decrease_balance

Contract: https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-bAsset-contracts/contracts/anchor_basset_reward/src/user.rs#L80 https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-bAsset-contracts/contracts/anchor_basset_reward/src/user.rs#L125

Recommendation: In both the function add a check for amount!=0