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

modifier balancedBooks missing in a few functions #23

Closed
code423n4 opened this issue Jun 14, 2021 · 4 comments
Closed

modifier balancedBooks missing in a few functions #23

code423n4 opened this issue Jun 14, 2021 · 4 comments
Labels
2 (Med Risk) bug Something isn't working duplicate This issue or pull request already exists resolved sponsor confirmed

Comments

@code423n4
Copy link
Contributor

Handle

gpersoon

Vulnerability details

Impact

Most of the functions of RCTreasury.sol, that manipulate totalDeposits, marketBalance or totalMarketPots use the modifier balancedBooks.
However the functions refundUser and topupMarketBalance don't use the modifier.
It doesn't hurt to add the extra safeguard.

Proof of Concept

// https://github.com/code-423n4/2021-06-realitycards/blob/main/contracts/RCTreasury.sol#L124
/// @notice check that funds haven't gone missing during this function call
modifier balancedBooks {
_;
// using >= not == in case anyone sends tokens direct to contract
require(erc20.balanceOf(address(this)) >= totalDeposits + marketBalance + totalMarketPots,"Books are unbalanced!");
}

// https://github.com/code-423n4/2021-06-realitycards/blob/main/contracts/RCTreasury.sol#L447
function refundUser(address _user, uint256 _refund) external override onlyMarkets {
marketBalance -= _refund;
...
totalDeposits += _refund;

// https://github.com/code-423n4/2021-06-realitycards/blob/main/contracts/RCTreasury.sol#L372
function topupMarketBalance(uint256 _amount) external override {
....
marketBalance += _amount;
}

Tools Used

Recommended Mitigation Steps

Add the modifier balancedBooks to the functions refundUser and topupMarketBalance

@mcplums
Copy link
Collaborator

mcplums commented Jun 18, 2021

This is a good one- thanks @gpersoon :)

@Splidge
Copy link
Collaborator

Splidge commented Jun 21, 2021

implemented here

@dmvt
Copy link
Collaborator

dmvt commented Jul 9, 2021

I agree with @0xRajeev in #112 that this should be medium severity. The likelihood is low but the potential impact could be quite high.

@dmvt dmvt added 2 (Med Risk) duplicate This issue or pull request already exists and removed 1 (Low Risk) labels Jul 9, 2021
@dmvt
Copy link
Collaborator

dmvt commented Jul 11, 2021

duplicate of #112

@dmvt dmvt closed this as completed Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) bug Something isn't working duplicate This issue or pull request already exists resolved sponsor confirmed
Projects
None yet
Development

No branches or pull requests

4 participants