Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #59

Open
code423n4 opened this issue Mar 9, 2022 · 1 comment
Open

QA Report #59

code423n4 opened this issue Mar 9, 2022 · 1 comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

C4-001 : PREVENT DIV BY 0

Impact - LOW

On several locations in the code precautions are taken not to divide by 0, because this will revert the code. However on some locations this isn’t done.

Oracle price is not checked. That will cause to revert on the several functions.

Proof of Concept

  1. Navigate to the following contract.

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/oracle/src/contract.rs#L172

Tools Used

None

Recommended Mitigation Steps

Recommend making sure division by 0 won’t occur by checking the variables beforehand and handling this edge case.

C4-002 : Coins can be lost which is not white-listed stable coin

Impact - LOW

Transmitted coins are filtered by the configured stable denomination in multiple places across the codebase, however any other coins sent are not returned to the sender. That leaves those other coins frozen to the contract.

Proof of Concept

  1. Navigate to the following contract.

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-bAsset-contracts/contracts/anchor_basset_hub/src/contract.rs#L43

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/anchor-bAsset-contracts/contracts/anchor_basset_hub/src/bond.rs#L41

Tools Used

None

Recommended Mitigation Steps

Consider returning unused coins or reverting the transaction if unexpected coins
are sent.

C4-003 : Initialize Function Does Not Have Enough Sanity Check

Impact - LOW

The Overseer contract is responsible for storing key protocol parameters and the whitelisting of new bAsset collaterals. The Overseer keeps track of locked collateral amounts and calculates the borrow limits for each user. The initialize function does not have any sanity check in the smart contract variables.

Proof of Concept

  1. Navigate to the following contract.

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/overseer/src/contract.rs#L46

Tools Used

None

Recommended Mitigation Steps

The protocol considers to the variable is between 0 and 1. However, the sanity checks are not implemented. That will cause to arithmetic errors.

C4-004 : Missing zero-address check in the setter functions and initialize functions

Impact - LOW

Missing checks for zero-addresses may lead to broken functionality in the protocol, if the variable addresses are updated incorrectly.

Proof of Concept

  1. Navigate to the following contracts.
https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/cross-chain-contracts/ethereum/CrossAnchorBridge.sol#L152

Tools Used

Code Review

Recommended Mitigation Steps

Consider adding zero-address checks in the discussed constructors:
require(newAddr != address(0));.

C4-005 : Previous Feeder Can Be Overwritten

Impact - LOW

The Oracle contract acts as the price source for the Anchor Money Market. Price sources are gaven as a resource with the feeders. With the register_feeder function, the existing feeder will be overwritten. Therefore, the code does not check existing feeder.

Proof of Concept

  1. Navigate to the following contracts.
https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/oracle/src/contract.rs#L76

Tools Used

Code Review

Recommended Mitigation Steps

Consider checking the existing feeder.

C4-006 : Empty String Can Be Inserted As a Whitelist

Impact - LOW

The Overseer contract is responsible for storing key protocol parameters and the whitelisting of new bAsset collaterals. The Overseer keeps track of locked collateral amounts and calculates the borrow limits for each user.

Proof of Concept

  1. Navigate to the following contracts.
https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/overseer/src/contract.rs#L245

Tools Used

Code Review

Recommended Mitigation Steps

Consider to trim and validate all String parameters before the state.

C4-007 : Critical changes should use two-step procedure

Impact - NON CRITICAL

The owner is the authorized user in the cosmwasm contracts. Usually, an owner can be updated with update_config function. However, the process is only completed with single transaction. If the address is updated incorrectly, an owner functionality will be lost forever.

Proof of Concept

  1. Navigate to the following contracts.
https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/oracle/src/contract.rs#L58

https://github.com/code-423n4/2022-02-anchor/blob/main/contracts/money-market-contracts/contracts/distribution_model/src/contract.rs#L79

Tools Used

Code Review

Recommended Mitigation Steps

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

@code423n4 code423n4 added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax bug Something isn't working labels Mar 9, 2022
code423n4 added a commit that referenced this issue Mar 9, 2022
@GalloDaSballo
Copy link
Collaborator

Whitelist, same as #44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

2 participants