Skip to content

Commit

Permalink
FUN-980 - Add additional Functions foundry units tests from audit
Browse files Browse the repository at this point in the history
  • Loading branch information
@justinkaseman
justinkaseman committed Oct 16, 2023
1 parent f74c15a commit c3c5c8c
Show file tree
Hide file tree
Showing 2 changed files with 131 additions and 22 deletions.
118 changes: 99 additions & 19 deletions contracts/src/v0.8/functions/tests/v1_X/FunctionsSubscriptions.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ contract FunctionsSubscriptions_OwnerCancelSubscription is FunctionsSubscription
string[] memory args = new string[](0);
bytes[] memory bytesArgs = new bytes[](0);

s_functionsClient.sendRequest(s_donId, sourceCode, secrets, args, bytesArgs, s_subscriptionId, 5000);
s_functionsClient.sendRequest(s_donId, sourceCode, secrets, args, bytesArgs, s_subscriptionId, 5500);
s_functionsRouter.ownerCancelSubscription(s_subscriptionId);
}

Expand Down Expand Up @@ -130,8 +130,12 @@ contract FunctionsSubscriptions_OwnerCancelSubscription is FunctionsSubscription
contract FunctionsSubscriptions_RecoverFunds is FunctionsRouterSetup {
event FundsRecovered(address to, uint256 amount);

function test_RecoverFunds_Success() public {
uint256 fundsTransferred = 1 * 1e18; // 1 LINK
function test_RecoverFunds_Success(uint64 fundsTransferred) public {
//amount must be less than LINK total supply
vm.assume(fundsTransferred < 1_000_000_000 * 1e18);
vm.assume(fundsTransferred > 0);

// uint256 fundsTransferred = 1 * 1e18; // 1 LINK
s_linkToken.transfer(address(s_functionsRouter), fundsTransferred);

// topic0 (function signature, always checked), NOT topic1 (false), NOT topic2 (false), NOT topic3 (false), and data (true).
Expand Down Expand Up @@ -268,6 +272,14 @@ contract FunctionsSubscriptions_OwnerWithdraw is FunctionsFulfillmentSetup {
// s_functionsRouter.ownerWithdraw(OWNER_ADDRESS, amountToWithdraw);
}

function test_OwnerWithdraw_SuccessIfRecipientAddressZero() public {
uint256 balanceBefore = s_linkToken.balanceOf(address(0));
uint96 amountToWithdraw = s_fulfillmentRouterOwnerBalance;
s_functionsRouter.ownerWithdraw(address(0), amountToWithdraw);
uint256 balanceAfter = s_linkToken.balanceOf(address(0));
assertEq(balanceBefore + s_fulfillmentRouterOwnerBalance, balanceAfter);
}

function test_OwnerWithdraw_SuccessIfNoAmount() public {
uint256 balanceBefore = s_linkToken.balanceOf(OWNER_ADDRESS);
uint96 amountToWithdraw = 0;
Expand Down Expand Up @@ -298,34 +310,49 @@ contract FunctionsSubscriptions_OwnerWithdraw is FunctionsFulfillmentSetup {

/// @notice #onTokenTransfer
contract FunctionsSubscriptions_OnTokenTransfer is FunctionsSubscriptionSetup {
function test_OnTokenTransfer_RevertIfPaused() public {
function test_OnTokenTransfer_RevertIfPaused(uint96 fundingAmount) public {
// Funding amount must be less than LINK total supply
vm.assume(fundingAmount < 1_000_000_000 * 1e18);
vm.assume(fundingAmount > 0);

s_functionsRouter.pause();
vm.expectRevert("Pausable: paused");
uint96 fundingAmount = 100;
s_linkToken.transferAndCall(address(s_functionsRouter), fundingAmount, abi.encode(s_subscriptionId));
}

function test_OnTokenTransfer_RevertIfCallerIsNotLink() public {
function test_OnTokenTransfer_RevertIfCallerIsNotLink(uint96 fundingAmount) public {
// Funding amount must be less than LINK total supply
vm.assume(fundingAmount < 1_000_000_000 * 1e18);
vm.assume(fundingAmount > 0);

vm.expectRevert(FunctionsSubscriptions.OnlyCallableFromLink.selector);
uint96 fundingAmount = 100;
s_functionsRouter.onTokenTransfer(address(s_functionsRouter), fundingAmount, abi.encode(s_subscriptionId));
}

function test_OnTokenTransfer_RevertIfCallerIsNoCalldata() public {
function test_OnTokenTransfer_RevertIfCallerIsNoCalldata(uint96 fundingAmount) public {
// Funding amount must be less than LINK total supply
vm.assume(fundingAmount < 1_000_000_000 * 1e18);
vm.assume(fundingAmount > 0);

vm.expectRevert(FunctionsSubscriptions.InvalidCalldata.selector);
uint96 fundingAmount = 100;
s_linkToken.transferAndCall(address(s_functionsRouter), fundingAmount, new bytes(0));
}

function test_OnTokenTransfer_RevertIfCallerIsNoSubscription() public {
function test_OnTokenTransfer_RevertIfCallerIsNoSubscription(uint96 fundingAmount) public {
// Funding amount must be less than LINK total supply
vm.assume(fundingAmount < 1_000_000_000 * 1e18);
vm.assume(fundingAmount > 0);

vm.expectRevert(FunctionsSubscriptions.InvalidSubscription.selector);
uint96 fundingAmount = 100;
uint64 invalidSubscriptionId = 123456789;
s_linkToken.transferAndCall(address(s_functionsRouter), fundingAmount, abi.encode(invalidSubscriptionId));
}

function test_OnTokenTransfer_Success() public {
uint96 fundingAmount = 100;
function test_OnTokenTransfer_Success(uint96 fundingAmount) public {
// Funding amount must be less than LINK total supply
vm.assume(fundingAmount < 1_000_000_000 * 1e18);
vm.assume(fundingAmount > 0);

uint96 subscriptionBalanceBefore = s_functionsRouter.getSubscription(s_subscriptionId).balance;
s_linkToken.transferAndCall(address(s_functionsRouter), fundingAmount, abi.encode(s_subscriptionId));
uint96 subscriptionBalanceAfter = s_functionsRouter.getSubscription(s_subscriptionId).balance;
Expand Down Expand Up @@ -541,13 +568,14 @@ contract FunctionsSubscriptions_CreateSubscriptionWithConsumer is FunctionsClien
assertEq(firstCallSubscriptionId, 1);
assertEq(s_functionsRouter.getSubscription(firstCallSubscriptionId).consumers[0], address(s_functionsClient));

// Consumer can be address(0)
vm.expectEmit(checkTopic1, checkTopic2, checkTopic3, checkData);
emit SubscriptionCreated(2, OWNER_ADDRESS);
vm.expectEmit(checkTopic1, checkTopic2, checkTopic3, checkData);
emit SubscriptionConsumerAdded(2, address(s_functionsClient));
uint64 secondCallSubscriptionId = s_functionsRouter.createSubscriptionWithConsumer(address(s_functionsClient));
emit SubscriptionConsumerAdded(2, address(0));
uint64 secondCallSubscriptionId = s_functionsRouter.createSubscriptionWithConsumer(address(0));
assertEq(secondCallSubscriptionId, 2);
assertEq(s_functionsRouter.getSubscription(secondCallSubscriptionId).consumers[0], address(s_functionsClient));
assertEq(s_functionsRouter.getSubscription(secondCallSubscriptionId).consumers[0], address(0));

vm.expectEmit(checkTopic1, checkTopic2, checkTopic3, checkData);
emit SubscriptionCreated(3, OWNER_ADDRESS);
Expand Down Expand Up @@ -579,20 +607,36 @@ contract FunctionsSubscriptions_CreateSubscriptionWithConsumer is FunctionsClien
contract FunctionsSubscriptions_ProposeSubscriptionOwnerTransfer is FunctionsSubscriptionSetup {
uint256 internal NEW_OWNER_PRIVATE_KEY_WITH_TOS = 0x3;
address internal NEW_OWNER_ADDRESS_WITH_TOS = vm.addr(NEW_OWNER_PRIVATE_KEY_WITH_TOS);
uint256 internal NEW_OWNER_PRIVATE_KEY_WITHOUT_TOS = 0x4;
uint256 internal NEW_OWNER_PRIVATE_KEY_WITH_TOS2 = 0x4;
address internal NEW_OWNER_ADDRESS_WITH_TOS2 = vm.addr(NEW_OWNER_PRIVATE_KEY_WITH_TOS2);
uint256 internal NEW_OWNER_PRIVATE_KEY_WITHOUT_TOS = 0x5;
address internal NEW_OWNER_ADDRESS_WITHOUT_TOS = vm.addr(NEW_OWNER_PRIVATE_KEY_WITHOUT_TOS);

function setUp() public virtual override {
FunctionsSubscriptionSetup.setUp();

// Accept ToS as new owner
// Accept ToS as new owner #1
vm.stopPrank();
vm.startPrank(NEW_OWNER_ADDRESS_WITH_TOS);
bytes32 message = s_termsOfServiceAllowList.getMessage(NEW_OWNER_ADDRESS_WITH_TOS, NEW_OWNER_ADDRESS_WITH_TOS);
bytes32 prefixedMessage = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", message));
(uint8 v, bytes32 r, bytes32 s) = vm.sign(TOS_SIGNER_PRIVATE_KEY, prefixedMessage);
s_termsOfServiceAllowList.acceptTermsOfService(NEW_OWNER_ADDRESS_WITH_TOS, NEW_OWNER_ADDRESS_WITH_TOS, r, s, v);

// Accept ToS as new owner #2
vm.stopPrank();
vm.startPrank(NEW_OWNER_ADDRESS_WITH_TOS2);
bytes32 message2 = s_termsOfServiceAllowList.getMessage(NEW_OWNER_ADDRESS_WITH_TOS2, NEW_OWNER_ADDRESS_WITH_TOS2);
bytes32 prefixedMessage2 = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", message2));
(uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(TOS_SIGNER_PRIVATE_KEY, prefixedMessage2);
s_termsOfServiceAllowList.acceptTermsOfService(
NEW_OWNER_ADDRESS_WITH_TOS2,
NEW_OWNER_ADDRESS_WITH_TOS2,
r2,
s2,
v2
);

vm.stopPrank();
vm.startPrank(OWNER_ADDRESS);
}
Expand Down Expand Up @@ -653,6 +697,24 @@ contract FunctionsSubscriptions_ProposeSubscriptionOwnerTransfer is FunctionsSub
s_functionsRouter.proposeSubscriptionOwnerTransfer(s_subscriptionId, NEW_OWNER_ADDRESS_WITH_TOS);
assertEq(s_functionsRouter.getSubscription(s_subscriptionId).proposedOwner, NEW_OWNER_ADDRESS_WITH_TOS);
}

function test_ProposeSubscriptionOwnerTransfer_SuccessChangeProposedOwner() public {
// topic0 (function signature, always checked), topic1 (true), NOT topic2 (false), NOT topic3 (false), and data (true).
bool checkTopic1 = true;
bool checkTopic2 = false;
bool checkTopic3 = false;
bool checkData = true;

vm.expectEmit(checkTopic1, checkTopic2, checkTopic3, checkData);
emit SubscriptionOwnerTransferRequested(s_subscriptionId, OWNER_ADDRESS, NEW_OWNER_ADDRESS_WITH_TOS);
s_functionsRouter.proposeSubscriptionOwnerTransfer(s_subscriptionId, NEW_OWNER_ADDRESS_WITH_TOS);
assertEq(s_functionsRouter.getSubscription(s_subscriptionId).proposedOwner, NEW_OWNER_ADDRESS_WITH_TOS);

vm.expectEmit(checkTopic1, checkTopic2, checkTopic3, checkData);
emit SubscriptionOwnerTransferRequested(s_subscriptionId, OWNER_ADDRESS, NEW_OWNER_ADDRESS_WITH_TOS2);
s_functionsRouter.proposeSubscriptionOwnerTransfer(s_subscriptionId, NEW_OWNER_ADDRESS_WITH_TOS2);
assertEq(s_functionsRouter.getSubscription(s_subscriptionId).proposedOwner, NEW_OWNER_ADDRESS_WITH_TOS2);
}
}

/// @notice #acceptSubscriptionOwnerTransfer
Expand Down Expand Up @@ -735,6 +797,13 @@ contract FunctionsSubscriptions_AcceptSubscriptionOwnerTransfer is FunctionsSubs
event SubscriptionOwnerTransferred(uint64 indexed subscriptionId, address from, address to);

function test_AcceptSubscriptionOwnerTransfer_Success() public {
// Can transfer ownership with a pending request
string memory sourceCode = "return 'hello world';";
bytes memory secrets;
string[] memory args = new string[](0);
bytes[] memory bytesArgs = new bytes[](0);
s_functionsClient.sendRequest(s_donId, sourceCode, secrets, args, bytesArgs, s_subscriptionId, 5500);

s_functionsRouter.proposeSubscriptionOwnerTransfer(s_subscriptionId, NEW_OWNER_ADDRESS_WITH_TOS);

// Send as new owner, who has accepted Terms of Service
Expand Down Expand Up @@ -1184,7 +1253,18 @@ contract FunctionsSubscriptions_SetFlags is FunctionsSubscriptionSetup {

/// @notice #getFlags
contract FunctionsSubscriptions_GetFlags is FunctionsSubscriptionSetup {
function test_GetFlags_Success() public {
function test_GetFlags_SuccessInvalidSubscription() public {
// Send as stranger
vm.stopPrank();
vm.startPrank(STRANGER_ADDRESS);

uint64 invalidSubscriptionId = 999999;

bytes32 flags = s_functionsRouter.getFlags(invalidSubscriptionId);
assertEq(flags, bytes32(0));
}

function test_GetFlags_SuccessValidSubscription() public {
// Set flags
bytes32 flagsToSet = bytes32("1");
s_functionsRouter.setFlags(s_subscriptionId, flagsToSet);
Expand Down
35 changes: 32 additions & 3 deletions contracts/src/v0.8/functions/tests/v1_X/FunctionsTermsOfServiceAllowList.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,26 @@ contract FunctionsTermsOfServiceAllowList_AcceptTermsOfService is FunctionsRoute
s_termsOfServiceAllowList.acceptTermsOfService(STRANGER_ADDRESS, address(s_functionsClientHelper), r, s, v);
}

function testAcceptTermsOfService_InvalidSigner_vuln() public {
// Set the signer as the zero address
TermsOfServiceAllowList.Config memory allowListConfig;
allowListConfig.enabled = true;
allowListConfig.signerPublicKey = address(0);
s_termsOfServiceAllowList.updateConfig(allowListConfig);

// Provide garbage data (v cannot be 29) to generate an invalid signature
uint8 v = 29;
bytes32 r = 0x0101010000000000000000000000000000000000000000000000000000000000;
bytes32 s = 0x0101010000000000000000000000000000000000000000000000000000000000;

// Expect a revert on invalid signature but the call is successful
vm.stopPrank();
vm.startPrank(STRANGER_ADDRESS);
// vm.expectRevert(TermsOfServiceAllowList.InvalidSignature.selector);
// TODO: Add validation to setConfig to prevent empty signer
s_termsOfServiceAllowList.acceptTermsOfService(STRANGER_ADDRESS, STRANGER_ADDRESS, r, s, v);
}

event AddedAccess(address user);

function test_AcceptTermsOfService_SuccessIfAcceptingForSelf() public {
Expand All @@ -175,7 +195,14 @@ contract FunctionsTermsOfServiceAllowList_AcceptTermsOfService is FunctionsRoute

s_termsOfServiceAllowList.acceptTermsOfService(STRANGER_ADDRESS, STRANGER_ADDRESS, r, s, v);

assertEq(s_termsOfServiceAllowList.hasAccess(STRANGER_ADDRESS, new bytes(0)), true);
assertTrue(s_termsOfServiceAllowList.hasAccess(STRANGER_ADDRESS, new bytes(0)));

// Event emitted even though adding existing item into EnumerableSet set does nothing
// TODO: handle differently in contract
vm.expectEmit(checkTopic1, checkTopic2, checkTopic3, checkData);
emit AddedAccess(STRANGER_ADDRESS);
s_termsOfServiceAllowList.acceptTermsOfService(STRANGER_ADDRESS, STRANGER_ADDRESS, r, s, v);
assertTrue(s_termsOfServiceAllowList.hasAccess(STRANGER_ADDRESS, new bytes(0)));
}

function test_AcceptTermsOfService_SuccessIfAcceptingForContract() public {
Expand Down Expand Up @@ -279,6 +306,8 @@ contract FunctionsTermsOfServiceAllowList_BlockSender is FunctionsRoutesSetup {
event BlockedAccess(address user);

function test_BlockSender_Success() public {
assertFalse(s_termsOfServiceAllowList.isBlockedSender(STRANGER_ADDRESS));

// topic0 (function signature, always checked), NOT topic1 (false), NOT topic2 (false), NOT topic3 (false), and data (true).
bool checkTopic1 = false;
bool checkTopic2 = false;
Expand All @@ -288,8 +317,8 @@ contract FunctionsTermsOfServiceAllowList_BlockSender is FunctionsRoutesSetup {
emit BlockedAccess(STRANGER_ADDRESS);

s_termsOfServiceAllowList.blockSender(STRANGER_ADDRESS);
assertEq(s_termsOfServiceAllowList.hasAccess(STRANGER_ADDRESS, new bytes(0)), false);
assertEq(s_termsOfServiceAllowList.isBlockedSender(STRANGER_ADDRESS), true);
assertFalse(s_termsOfServiceAllowList.hasAccess(STRANGER_ADDRESS, new bytes(0)));
assertTrue(s_termsOfServiceAllowList.isBlockedSender(STRANGER_ADDRESS));

// Account can no longer accept Terms of Service
bytes32 message = s_termsOfServiceAllowList.getMessage(STRANGER_ADDRESS, STRANGER_ADDRESS);
Expand Down

0 comments on commit c3c5c8c

Please sign in to comment.