MyCut
AI First Flight #8
Beginner FriendlyFoundry
EXP
View results
Previous Next
Submission Details
Impact: medium
Likelihood: medium
Invalid

[M-1] `ContestManager::getContestTotalRewards` does not check that param `address contract` is an approved contest, which returns a uint256 instead of reverting

datadata

[M-1] ContestManager::getContestTotalRewards does not check that param address contract is an approved contest, which returns a uint256 instead of reverting

Description

ContestManager::getContestTotalRewards does not check that param address contract is an approved contest, returning the default of uint256 which is 0, instead of reverting. This means that the information introduced into the protocol is not precise. The logic currently puts contests in array ContestManager::contests, which could be checked, although we recommend the use of a mapping to do this check. See mitigation section.

function getContestTotalRewards(
@> address contest
) public view returns (uint256) {
@> return contestToTotalRewards[contest];
}

Risk

Likelihood:

This happens any time a non-approved address is passed to ContestManager::getContestTotalRewards.

Impact:

The impact is that a given address is not checked as a valid contest, which means that the default value of uint256 is returned when the address is passed to mapping ContestManager::contestToTotalRewards instead of this function reverting. This introduces uncertainty into the protocol, because now it appears that the address might be a legitimate contest, but that it has no rewards remaining.

Proof of Concept

We define a fake contract DummyContract in the test file test/TestMyCut.t.sol.

contract DummyContract {}

In our test, we deploy a contract fakePot based on DummyContract, and we pass the address of fakePot to ContestManager::contestToTotalRewards. Instead of reverting, the function returns the default value of uint256 in the mapping, which is 0. This test function passes.

function testGetContestTotalRewardsDoesNotCheckAddress()
public
mintAndApproveTokens
{
// Arrange
// Deploy non-contest contract
DummyContract fakePot = new DummyContract();
​
// user deploys contest
vm.startPrank(user);
contest = ContestManager(conMan).createContest(
players,
rewards,
IERC20(ERC20Mock(weth)),
totalRewards
);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
// Act
// pass fakePot's address to ContestManager::getContestTotalRewards
uint256 myTotalRewards = ContestManager(conMan).getContestTotalRewards(
address(fakePot)
);
console.log("myTotalRewards: ", myTotalRewards);
​
// Assert
uint256 expectedFakeTotalRewards = 0;
assertEq(myTotalRewards, expectedFakeTotalRewards);
}

Recommended Mitigation

We recommend that a mapping of addresses to booleans be created to define if an address is a contest in ContestManager.

+ mapping (address => bool) public addressToContestBool;

The mapping will allow for constant-time look-up to verify that a given address is a contest. This mapping can be updated in ContestManager::createContest:

+ addressToContestBool[(address(pot))] = true;

We recommend that a custom error be added to ContestManager:

+ error ContestManager__addressIsNotAContest();

We recommend that the mapping be checked, and if the check does not return true, the custom error be thrown.

function getContestTotalRewards(
address contest
) public view returns (uint256) {
+ if (!addresstoContestBool[contest]) {
+ revert ContestManager__addressIsNotAContest();
+ }
return contestToTotalRewards[contest];
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 44 minutes ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!