Description: The protocol logic is when two users like each other, the ether they put for liking each other
should be combined and sent to the multi-signature wallet. However, the userBalances for different likes are stored in the same variable, causing the first match to drain the entire balance. This results in Ether being misallocated to the wrong multi-signature wallet.
Impact: The protocol logic is broken, and the ether is misallocated to the wrong multi-signature wallet.
Proof of Concept:
first need to fix another critical issue msg.value is not added to userBalances in likeUser:LikeRegistry
function likeUser(address liked) external payable{
require(msg.value >= 1 ether, "Must send at least 1 ETH");
require(!likes[msg.sender][liked], "Already liked");
require(msg.sender != liked, "Cannot like yourself");
require(profileNFT.profileToToken(msg.sender) != 0, "Must have a profile NFT");
require(profileNFT.profileToToken(liked) != 0, "Liked user must have a profile NFT");
+ userBalances[msg.sender] += msg.value;
likes[msg.sender][liked] = true;
emit Liked(msg.sender, liked);
create a test file LikeRegistryTest.t.sol with the following code
contract LikeRegistryTest is Test {
SoulboundProfileNFT soulboundNFT;
LikeRegistry likeRegistry;
LikeRegistryEdit likeRegistryEdit;
address ownerOfNFT = makeAddr("ownerOfNFT");
address ownerOfLike = makeAddr("ownerOfLike");
address bob = makeAddr("bob");
address alice = makeAddr("alice");
address miriam = makeAddr("miriam");
address roxas = makeAddr("roxas");
uint256 constant INITIAL_BALANCE = 100 ether;
function setUp() public {
vm.prank(ownerOfNFT);
soulboundNFT = new SoulboundProfileNFT();
vm.prank(ownerOfLike);
likeRegistry = new LikeRegistry(address(soulboundNFT));
vm.prank(bob);
soulboundNFT.mintProfile("Bob", 25, "ipfs://bobPhoto");
vm.prank(alice);
soulboundNFT.mintProfile("Alice", 23, "ipfs://alicePhoto");
vm.prank(miriam);
soulboundNFT.mintProfile("Miriam", 16, "ipfs://miriamPhoto");
vm.deal(bob, INITIAL_BALANCE);
vm.deal(alice, INITIAL_BALANCE);
vm.deal(miriam, INITIAL_BALANCE);
vm.deal(roxas, INITIAL_BALANCE);
vm.prank(ownerOfLike);
likeRegistryEdit = new LikeRegistryEdit(address(soulboundNFT));
}
...
function testUserBalanceIsMisplaced() public {
vm.startPrank(bob);
likeRegistryEdit.likeUser{value: 1 ether}(alice);
likeRegistryEdit.likeUser{value: 2 ether}(miriam);
vm.stopPrank();
vm.recordLogs();
vm.prank(alice);
likeRegistryEdit.likeUser{value: 1 ether}(bob);
vm.prank(miriam);
likeRegistryEdit.likeUser{value: 1 ether}(bob);
Vm.Log[] memory logs = vm.getRecordedLogs();
assertEq(logs.length, 6);
address bobAliceWalletAddress = address(uint160(uint256(bytes32(logs[2].data))));
address bobMiriamWalletAddress = address(uint160(uint256(bytes32(logs[5].data))));
assertEq(bobAliceWalletAddress.balance, (1 ether + 2 ether + 1 ether) * 9 / 10);
assertEq(bobMiriamWalletAddress.balance, 1 ether * 9 / 10);
}
run the test and the assertion indicates that the value bob likes miriam is misplaced to alice wallet
Recommended Mitigation:
change the userBalances from mapping(address => uint256) to mapping(address => mapping(address => uint256))
so it it indexed by the user and the liked user
then change the following code
function likeUser(address liked) external payable{
require(msg.value >= 1 ether, "Must send at least 1 ETH");
require(!likes[msg.sender][liked], "Already liked");
require(msg.sender != liked, "Cannot like yourself");
require(profileNFT.profileToToken(msg.sender) != 0, "Must have a profile NFT");
require(profileNFT.profileToToken(liked) != 0, "Liked user must have a profile NFT");
+ userBalances[msg.sender][liked] += msg.value;
likes[msg.sender][liked] = true;
emit Liked(msg.sender, liked);
function matchRewards(address from, address to) internal {
- uint256 matchUserOne = userBalances[from];
- uint256 matchUserTwo = userBalances[to];
- userBalances[from] = 0;
- userBalances[to] = 0;
+ uint256 matchUserOne = userBalances[from][to];
+ uint256 matchUserTwo = userBalances[to][from];
+ userBalances[from][to] = 0;
+ userBalances[to][from] = 0;
