Summary
callReward function does not properly calculate the amountRewards of the HealthToken based on the MartenitsaTokens of users at that time. It should allow users to get 1 HelthToken every 3 new MartenitsaTokens but it does not take into account that the CountMartenitsaTokensOwner can change making or receiving MartenitsaTokens using the makePresent function.
Vulnerability Details
The calculation of the amountRewards in the codebase does not differentiate the new MartenitsaTokens from the old ones.
uint256 amountRewards = (count / requiredMartenitsaTokens) - _collectedRewards[msg.sender];
if (amountRewards > 0) {
_collectedRewards[msg.sender] = amountRewards;
healthToken.distributeHealthToken(msg.sender, amountRewards);
}
Impact
Considering a situation in which:
Bob buys 6 MartenitsaTokens and got 2 HealthTokens’ using the callRewardfunction. Then using themakePresentfunction, he transfers 3MartenisaTokensto Jack, and receives using the same function, 3 other newMartenisaTokensfrom Sam. Then Bob should be able to use again the functioncallRewardto get 1 moreHealthTokenfor the 3 newMartenisaTokens` but he can not get any additional token.
Run the test using command: forge test --match-path test/MartenitsaMarketplace.t.sol --match-test testReward -vvvv
function testReward() public listMartenitsa {
vm.startPrank(jack);
martenitsaToken.createMartenitsa("good1");
martenitsaToken.createMartenitsa("good2");
martenitsaToken.createMartenitsa("good3");
martenitsaToken.createMartenitsa("good4");
martenitsaToken.createMartenitsa("good5");
marketplace.listMartenitsaForSale(1, 1 wei);
marketplace.listMartenitsaForSale(2, 1 wei);
marketplace.listMartenitsaForSale(3, 1 wei);
marketplace.listMartenitsaForSale(4, 1 wei);
marketplace.listMartenitsaForSale(5, 1 wei);
martenitsaToken.setApprovalForAll(address(marketplace), true);
vm.stopPrank();
vm.startPrank(chasy);
martenitsaToken.createMartenitsa("good6");
martenitsaToken.createMartenitsa("good7");
martenitsaToken.createMartenitsa("good8");
marketplace.listMartenitsaForSale(6, 1 wei);
marketplace.listMartenitsaForSale(7, 1 wei);
marketplace.listMartenitsaForSale(8, 1 wei);
martenitsaToken.setApprovalForAll(address(marketplace), true);
vm.stopPrank();
vm.startPrank(bob);
marketplace.buyMartenitsa{value: 1 wei}(0);
marketplace.buyMartenitsa{value: 1 wei}(1);
marketplace.buyMartenitsa{value: 1 wei}(2);
marketplace.buyMartenitsa{value: 1 wei}(3);
marketplace.buyMartenitsa{value: 1 wei}(4);
marketplace.buyMartenitsa{value: 1 wei}(5);
marketplace.collectReward();
martenitsaToken.setApprovalForAll(address(marketplace), true);
marketplace.makePresent(jack, 3);
marketplace.makePresent(jack, 4);
marketplace.makePresent(jack, 5);
vm.stopPrank();
vm.startPrank(sam);
marketplace.buyMartenitsa{value: 1 wei}(6);
marketplace.buyMartenitsa{value: 1 wei}(7);
marketplace.buyMartenitsa{value: 1 wei}(8);
marketplace.collectReward();
martenitsaToken.setApprovalForAll(address(marketplace), true);
marketplace.makePresent(bob, 6);
marketplace.makePresent(bob, 7);
marketplace.makePresent(bob, 8);
vm.stopPrank();
vm.startPrank(bob);
marketplace.collectReward();
assert(healthToken.balanceOf(bob) == 2 * 10 ** 18);
vm.stopPrank();
}
and you get the following result:
├─ [0] VM::stopPrank()
│ └─ ← ()
├─ [0] VM::startPrank(bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e])
│ └─ ← ()
├─ [2889] MartenitsaMarketplace::collectReward()
│ ├─ [630] MartenitsaToken::isProducer(bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e]) [staticcall]
│ │ └─ ← false
│ ├─ [627] MartenitsaToken::getCountMartenitsaTokensOwner(bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e]) [staticcall]
│ │ └─ ← 6
│ └─ ← ()
├─ [629] HealthToken::balanceOf(bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e]) [staticcall]
│ └─ ← 2000000000000000000 [2e18]
├─ [0] VM::stopPrank()
│ └─ ← ()
└─ ← ()
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.15ms
Ran 1 test suite in 3.15ms: 1 tests passed, 0 failed, 0 skipped (1 total tests)
Tools Used
Foundry
Recommendations
Make these changes to the codebase:
mapping(address => uint256) private _collectedRewards;
mapping(uint256 => Listing) public tokenIdToListing;
+ mapping(address => mapping(uint256 => bool)) public IdTokenusedforrewards;
function collectReward() external {
require(!martenitsaToken.isProducer(msg.sender), "You are producer and not eligible for a reward!");
uint256 count = martenitsaToken.getCountMartenitsaTokensOwner(msg.sender);
- uint256 amountRewards = (count / requiredMartenitsaTokens) - _collectedRewards[msg.sender];
- if (amountRewards > 0) {
- _collectedRewards[msg.sender] = amountRewards;
- healthToken.distributeHealthToken(msg.sender, amountRewards);
- }
- }
+ uint256 nexttokenid = martenitsaToken.getNextTokenId();
+ for (uint256 i=0; i<nexttokenid; ++i){
+ if (msg.sender == martenitsaToken.ownerOf(i) && IdTokenusedforrewards[msg.sender][i] == true){
+ count -= 1;
+ }
+ else if (msg.sender == martenitsaToken.ownerOf(i) && IdTokenusedforrewards[msg.sender][i] == false){
+ IdTokenusedforrewards[msg.sender][i] = true;
+ }
+ }
+ uint256 amountRewards = (count / requiredMartenitsaTokens);
+ healthToken.distributeHealthToken(msg.sender, amountRewards);
+ }
