First Flight #5: Santa's List
First Flight #5
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Cost of a Present Is Not Correctly Deducted From User

atlanticbase

Summary

Due to the wrong arguments passed in, the caller gets the NFT and the receiver that is passed in to the argument is the one buying for the caller instead. This could be abused and is not the intended functionality.

Vulnerability Details

In buyPresent, the wrong address is passed in to the argument to the burn function. Instead of msg.sender, which logically should bear the cost of the transaction as they are initiating the purchase for another user, the function erroneously charges the receiver address. This is further amplified as the burn function does not require the receiver address to approve. The caller can call on any receiver that has enough tokens and he could receive a free NFT without spending any tokens.

Moreover, there is a discrepancy between the documented and actual token cost for purchasing a present. While the documentation states a cost of 2e18 tokens, the function in practice only burns 1e18 tokens.

Instead of burning the defined cost in SantasList.sol, it only burns 1e18, as seen here.

function burn(address from) external {
if (msg.sender != i_santasList) {
revert SantaToken__NotSantasList();
}
_burn(from, 1e18);
}

POC

function testBuyPresentForOthersNotDepletingOwnTokens() public {
address alice = makeAddr("alice");
address bob = makeAddr("bob");
​
// Give alice 1e18 tokens
deal(address(santaToken), alice, 1 ether);
​
console.log("alice SantaToken balance: ", santaToken.balanceOf(alice));
console.log("bob SantaToken balance: ", santaToken.balanceOf(bob));
​
// bob buys for alice
vm.startPrank(bob);
santasList.buyPresent(alice);
​
console.log("alice SantaToken balance: ", santaToken.balanceOf(alice));
console.log("bob SantaToken balance: ", santaToken.balanceOf(bob));
}
Running 1 test for test/unit/SantasListTest.t.sol:SantasListTest
[PASS] testBuyPresentForOthersNotDepletingOwnTokens() (gas: 235475)
Logs:
alice SantaToken balance: 1000000000000000000
bob SantaToken balance: 0
alice SantaToken balance: 0
bob SantaToken balance: 0

Impact

User can buy as many presents as possible without spending any of his own funds. The present cost is also incorrect as it only burns 1e18 from the user instead of the defined cost of 2e18 in the documentation.

Tools Used

Manual Review

Recommendations

Consider changing the address to msg.sender and adding an address parameters to _mintAndIncrement().

function buyPresent(address presentReceiver) external {
- i_santaToken.burn(presentReceiver);
+ i_santaToken.burn(msg.sender);
_mintAndIncrement();
}
- function _mintAndIncrement() private {
+ function _mintAndIncrement(address _to) private {
- _safeMint(msg.sender, s_tokenCounter++);
+. _safeMint(_to, s_tokenCounter++);
}

Also consider adding an amount parameter to the burn function in SantaToken as it is not costing the correct amount as stated in the docs. Right now, it is only burning 1e18 which is not the defined amount as stated in the docs which is 2e18.

- function burn(address from) external {
+ function burn(address from, uint256 amount) external {
if (msg.sender != i_santasList) {
revert SantaToken__NotSantasList();
}
- _burn(from, 1e18);
+ _burn(from, amount);
}
function buyPresent(address presentReceiver) external {
- i_santaToken.burn(presentReceiver);
+ i_santaToken.burn(presentReceiver, PURCHASED_PRESENT_COST);
_mintAndIncrement();
}
Updates

Lead Judging Commences

inallhonesty Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

buyPresent should use msg.sender

Current implementation allows a malicious actor to burn someone else's tokens as the burn function doesn't actually check for approvals.

Price is not enforced in buyPresent

This line indicates that the intended cost of presents for naughty people should be 2e18: https://github.com/Cyfrin/2023-11-Santas-List/blob/6627a6387adab89ae2ba2e82b38296723261c08a/src/SantasList.sol#L87 PURCHASE_PRESENT_COST should be implemented to enforce the cost of presents.

buyPresent should send to presentReceiver

Support

FAQs

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