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

Inaccurate return value from getCDS() possible #155

Open
code423n4 opened this issue Jan 11, 2022 · 2 comments
Open

Inaccurate return value from getCDS() possible #155

code423n4 opened this issue Jan 11, 2022 · 2 comments
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Something isn't working sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Handle

sirhashalot

Vulnerability details

Impact

The getCDS() function in Registry.sol may return an unexpected value when cds[_address] == address(0). The return value for the case of cds[_address] == address(0) is cds[address(0)], but cds[address(0)] can be set to a non-default value in the setCDS() function. Returning cds[address(0)] may return an address instead of returning address(0) or another value indicating there is no CDS for this address.

This issue is due to either a lack of a zero address check in the setCDS() function or a typo, it is unclear which.

Proof of Concept

The issue is this if statement branch in Registry.sol.

Recommended Mitigation Steps

A few solutions exist:

  1. The setCDS() function should add a require statement like require(_address != address(0)) to include a zero address check to prevent cds[0] from being set to a non-default value
  2. The getCDS() function should use return address(0) instead of return cds[address(0)];. If this solution is chosen, the if/else statement can be removed entirely to save gas so that the only line of code in this function is return cds[_address];
  3. If the goal of the if (cds[_address] == address(0)) check is to return a default value if no custom value has been set, then a separate variable containing a default value would be better than relying on the existing array.
@code423n4 code423n4 added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working labels Jan 11, 2022
code423n4 added a commit that referenced this issue Jan 11, 2022
@oishun1112
Copy link
Collaborator

This is our style of setting default value.
we set default value on mapping(ZERO_ADDRESS => defaultValue), but arbitrary value can be set on a specific address as well.

if (cds[_address] == address(0)) {
    return cds[address(0)];
} else {
    return cds[_address];
}

When nothing is set on the address, return the default value.
When something is set on the address, return it.

@oishun1112 oishun1112 added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Jan 18, 2022
@0xean
Copy link
Collaborator

0xean commented Jan 27, 2022

going to leave this as non-critical since it's a code clarity issue

``
0 — Non-critical: Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas-optimisations.

@0xean 0xean added 0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation and removed 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments labels Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Something isn't working sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants