Lack of access control in requestDeposit
Description
The BasketToken contract manages users' deposits and redemptions. It follows the ERC-7540 standard, which is an ERC-4626 vault with asynchronous operations. The requestDeposit function facilitates deposits of the base assets into the vault. The function stores the deposit request in the _depositRequests mapping and transfers the assets to the contract.
function requestDeposit(uint256 assets, address controller, address owner) public returns (uint256 requestId) {
// Checks
if (assets == 0) {
revert Errors.ZeroAmount();
}
requestId = nextDepositRequestId;
uint256 userLastDepositRequestId = lastDepositRequestId[controller];
// If the user has a pending deposit request in the past, they must wait for it to be fulfilled before making a
// new one
if (userLastDepositRequestId != requestId) {
if (pendingDepositRequest(userLastDepositRequestId, controller) > 0) {
revert MustClaimOutstandingDeposit();
}
}
// If the user has a claimable deposit request, they must claim it before making a new one
if (claimableDepositRequest(userLastDepositRequestId, controller) > 0) {
revert MustClaimOutstandingDeposit();
}
if (AssetRegistry(assetRegistry).hasPausedAssets(bitFlag)) {
revert AssetPaused();
}
// Effects
DepositRequestStruct storage depositRequest = _depositRequests[requestId];
// update controllers balance of assets pending deposit
depositRequest.depositAssets[controller] += assets;
// update total pending deposits for the current requestId
depositRequest.totalDepositAssets += assets;
// update controllers latest deposit request id
lastDepositRequestId[controller] = requestId;
emit DepositRequest(controller, owner, requestId, msg.sender, assets);
// Interactions
// Assets are immediately transferrred to here to await the basketManager to pull them
// slither-disable-next-line arbitrary-send-erc20
IERC20(asset()).safeTransferFrom(owner, address(this), assets);
}We note that the function does not implement any sort of access control with regards to the owner address. This means that any address can deposit assets on behalf of any other address, as long as the owner has previously approved the contract to spend their assets. This is generally the case in such contracts, as the approve function is called with type(uint256).max to allow the contract to spend any amount of the asset, improving the UX.
Impact
This would lead to total loss of base-asset funds for the users that have approved the contract to spend their assets. An attacker can deposit the assets of any user into the contract and specify themselves as the controller, effectively stealing the assets of the users.
Recommendations
We recommend ensuring that adequate approval checks are in place to prevent unauthorized deposits on behalf of users. Alternatively, only allow the owner to deposit assets on their own behalf.
Remediation
This issue has been acknowledged by Storm Labs, and a fix was implemented in commit f2d21812↗.