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

Use of assert() instead of require() #83

Open
code423n4 opened this issue Jun 16, 2021 · 1 comment
Open

Use of assert() instead of require() #83

code423n4 opened this issue Jun 16, 2021 · 1 comment
Labels
1 (Low Risk) bug Something isn't working

Comments

@code423n4
Copy link
Contributor

Handle

0xRajeev

Vulnerability details

Impact

Contracts use assert() instead of require() in multiple places. This causes a Panic error on failure and prevents the use of error strings.

Prior to solc 0.8.0, assert() used the invalid opcode which used up all the remaining gas while require() used the revert opcode which refunded the gas and therefore the importance of using require() instead of assert() was greater. However, after 0.8.0, assert() uses revert opcode just like require() but creates a Panic(uint256) error instead of Error(string) created by require(). Nevertheless, Solidity’s documentation says:

"Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.”

whereas

“The require function either creates an error without any data or an error of type Error(string). It should be used to ensure valid conditions that cannot be detected until execution time. This includes conditions on inputs or return values from calls to external contracts.”

Also, you can optionally provide a message string for require, but not for assert.

Proof of Concept

https://docs.soliditylang.org/en/v0.8.4/control-structures.html#panic-via-assert-and-error-via-require

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCFactory.sol#L388

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCFactory.sol#L437

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCMarket.sol#L202

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCMarket.sol#L367

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCMarket.sol#L551

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCMarket.sol#L731

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCMarket.sol#L797

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCMarket.sol#L1095

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCTreasury.sol#L437

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCTreasury.sol#L709

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCTreasury.sol#L735

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use require() with informative error strings instead of assert().

@code423n4 code423n4 added 1 (Low Risk) bug Something isn't working labels Jun 16, 2021
code423n4 added a commit that referenced this issue Jun 16, 2021
@Splidge Splidge added the duplicate This issue or pull request already exists label Jun 18, 2021
@Splidge
Copy link
Collaborator

Splidge commented Jun 18, 2021

Duplicate of #155

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 (Low Risk) bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants