Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

No access control for `CapitalPool::approve` allowing anyone to approve tokens

spomaria

Summary

The CapitalPool::approve function approves tokens for TokenManager.sol to transfer from one address to the other.

Vulnerability Details

The CapitalPool::approve function is only supposed to be called by the TokenManager.sol as specied in the natspec

/* @dev Approve token for token manager
@> * @notice only can be called by token manager
* @param tokenAddr address of token
*/
function approve(address tokenAddr) external {
address tokenManager = tadleFactory.relatedContracts(
RelatedContractLibraries.TOKEN_MANAGER
);
(bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
type(uint256).max
)
);
​
if (!success) {
revert ApproveFailed();
}
}

Notice that the function is marked external with no access control which means it can be called by any other contract.

Impact

Because the CapitalPool::approve function does not have any access controls, a malicious actor can approve a weird ERC20 token into the protocol.

Tools Used

Manual Review and Foundry

Proof of Concept:

  1. A malicious user creates a weird ERC20 token and a smart contract

  2. The malicious user's contract calls the CapitalPool::approve passing the weird ERC20 token to be approved.

PoC Place the following code outside `PreMarketsTest` contract in `PreMarkets.t.sol`.
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
.
.
.
contract MaliciousContract {
​
}
​
contract WeirdERC20 is ERC20 {
constructor() ERC20("Weird", "W20") {}
}

Now place the following code inside PreMarketsTest contract in PreMarkets.t.sol.

function test_CapitalPool_Approve_CanBeCalledByAnyone() public {
MaliciousContract malContract = new MaliciousContract();
WeirdERC20 weirdERC20 = new WeirdERC20();
​
vm.prank(address(malContract));
(bool success, ) = address(capitalPool).call(abi.encodeWithSignature("approve(address)", address(weirdERC20)));
​
assert(success);
}

Run: forge test --match-test test_CapitalPool_Approve_CanBeCalledByAnyone

Output:

Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[PASS] test_CapitalPool_Approve_CanBeCalledByAnyone() (gas: 534837)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 21.43ms (3.62ms CPU time)

Recommendations

  1. Consider modifying the CaptialPool::approve function to check if the caller is TokenManager.sol.

+ error Unauthorized();
​
.
.
.
function approve(address tokenAddr) external {
address tokenManager = tadleFactory.relatedContracts(
RelatedContractLibraries.TOKEN_MANAGER
);
​
+ if (_msgSender() != tokenManager) {
+ revert Unauthorized();
+ }
(bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
type(uint256).max
)
);
​
if (!success) {
revert ApproveFailed();
}
}

This will ensure that only TokenManager.sol can call the CapitalPool::approve function and thereby approve tokens on the protocol.

Updates

Lead Judging Commences

0xnevi Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-CapitalPool-approve-missing-access-control

This is at most low severity, even though giving max approvals shouldn't be permisionless, the respective tokenManager address is retrieved from the TadleFactory contract whereby the trusted guardian role is responsible for deploying such contracts as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/factory/TadleFactory.sol#L68). Since the user still has to go through the PreMarkets/DeliveryPlace contracts to perform market actions, this max approval cannot be exploited.

Support

FAQs

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