Anyone can call VVVVCTokenDistribution::claim function by utilizing ClaimParams signed by the signer
Let's get simple. First principles, right? The protocol or the codebase is not that important at this point, it is difficult to understand the nature of the bug when it's so much entangled with the protocol, so, I'll try to strip it down from the whole codebase, show it in its barenaked form as much as I can.
## The shallow waters
If I ever have children, there will be a strong temptation to name them Alice and Bob. So we have Alice and Bob. Alice has invested some money in Bob's business, and now she wants to claim her earnings. Bob gives Alice a check, a piece of paper that dictates that only Alice can claim her money, no one else. That check is signed, it is a signature. That's how it works in the world of smart contracts, as far as I'm concerned.
We can check that signature with the following function:
/**
* @notice Checks if the provided signature is valid
*
* @param _params A ClaimParams struct containing the investment parameters
*
* @return true if the signer address is recovered from the signature, false otherwise
*/
function _isSignatureValid(ClaimParams memory _params) private view returns (bool) {
bytes32 digest = keccak256(
abi.encodePacked(
"\x19\x01",
DOMAIN_SEPARATOR,
keccak256(
abi.encode(
CLAIM_TYPEHASH,
_params.kycAddress,
_params.projectTokenAddress,
_params.projectTokenProxyWallets,
_params.tokenAmountsToClaim,
_params.nonce,
_params.deadline
)
)
)
);
address recoveredAddress = ECDSA.recover(digest, _params.signature);
bool isSigner = recoveredAddress == signer;
bool isExpired = block.timestamp > _params.deadline;
return isSigner && !isExpired;
}
This function in itself has its own problems related to EIP712. To make a detour very, very briefly, the encoding is faulty. Why? Here's the struct for the params that is passed to the above verification function:
struct ClaimParams {
address kycAddress;
address projectTokenAddress;
address[] projectTokenProxyWallets;
uint256[] tokenAmountsToClaim;
uint256 nonce;
uint256 deadline;
bytes signature;
}
You see that projectTokenProxyWallets and tokenAmountsToClaim are arrays. According to EIP712, arrays should be encoded themselves before being passed to the encoding process.
So instead of passing this:
_params.projectTokenProxyWallets,
_params.tokenAmountsToClaim,
The code should've passed this:
keccak256(abi.encodePacked(_params.projectTokenProxyWallets)),
keccak256(abi.encodePacked(_params.tokenAmountsToClaim)),
But that's not the bug I missed. This is the bug I submitted. And at the time of writing, it is on its way to get invalidated by the community because the protocol did not specify that they follow EIP712 strictly. That is not our concern here. We're interested in THE BUG I DID NOT SEE.
## Missing the target
Now that we know about how the signature is verified, i.e. how the system, bank, or whatever knows what Alice is entitled to, let's see how she can get what she deserves. Here's te claim function. With this, imagine, Alice goes to the bank and gets her money:
function claim(ClaimParams memory _params) public {
if (claimIsPaused) {
revert ClaimIsPaused();
}
if (_params.projectTokenProxyWallets.length != _params.tokenAmountsToClaim.length) {
revert ArrayLengthMismatch();
}
if (_params.nonce <= nonces[_params.kycAddress]) {
revert InvalidNonce();
}
if (!_isSignatureValid(_params)) {
revert InvalidSignature();
}
// update nonce
nonces[_params.kycAddress] = _params.nonce;
// define token to transfer
IERC20 projectToken = IERC20(_params.projectTokenAddress);
// transfer tokens from each wallet to the caller
for (uint256 i = 0; i < _params.projectTokenProxyWallets.length; i++) {
projectToken.safeTransferFrom(
_params.projectTokenProxyWallets[i], msg.sender, _params.tokenAmountsToClaim[i]
);
}
emit VCClaim(
_params.kycAddress,
_params.projectTokenAddress,
_params.projectTokenProxyWallets,
_params.tokenAmountsToClaim,
_params.nonce
);
}
Pretty solid function, checks everything, takes care of everything. Safe and sound- I THOUGHT.
### Someone is watching your every move
Without further ado, the problem is here in this code block:
// transfer tokens from each wallet to the caller
for (uint256 i = 0; i < _params.projectTokenProxyWallets.length; i++) {
projectToken.safeTransferFrom(
=> _params.projectTokenProxyWallets[i], msg.sender, _params.tokenAmountsToClaim[i]
);
}
Look at that safeTransferFrom call. It sends the tokens to msg.sender, the caller of this function. Due to my lack of understanding how signatures work at the moment of the audit, or lack of coffee or sleep, whatever, I did think yeah, sure, we've already checked the signature, so everything is okay, Alice went to the bank, showed her check, the pieve of paper that proves she is the true owner of the money, and now she's getting her money, nice.
NO, not so nice. Not at all. My visualisation was wrong, and this do not work like the physical world in the realm of smart contracts. The above claim code does not check is the caller, msg.sender is the person specified in the signature (kycAddress). So someone basically do the following: Monitor the mempool, see Alice calling the claim function and passing the params, the attacker sees the params, they contain all the information necessary to pass the checks. It is like showing fake piece of papers to the guards. The attacker would fail, if the code was actually safe, when they are asked for identification. Since they don't possess Alice's ID, they would be refused by the bank. But the function DOES NOT ask for that. They don't check the ID bro.
Attacker is watching the mempool, sees the transaction, copies the params passed by Alice, pays higher gas and moves in front of Alice, calls the function and gets all the money. Alice gets nothing. She can't recall the function too, the nonce is updated.
The problem could have been avoided with several ways.
We could have had a check like this one:
require(msg.sender == _params.kycAddress, "Sender not authorized to claim on behalf of KYC address");
The above code makes sure that whomever is calling the function, the msg.sender, is actually the owner of the check. So the attacker cannot claim Alice's money because he does not have her ID card.
Or, we could have changed the token allocation like so:
// transfer tokens from each wallet to the caller
for (uint256 i = 0; i < _params.projectTokenProxyWallets.length; i++) {
projectToken.safeTransferFrom(
// we pass _params.kycAddress instead of msg.sender,
_params.projectTokenProxyWallets[i],
_params.kycAddress,
_params.tokenAmountsToClaim[i]
);
}
This works too. Now, even if the attacker is watching the mempool, he can't get Alice's money because when the attacker calls the function even with the params they watched and got from the mempool, the money will be send to the kycAddress, that is the address of Alice.
This kind of attack factors are called frontrunning. I couldn't imagine this in my physical world scenario. Someone who's watching Alice couldn't teleport in front of them, and get her money from the bank teller just by showing a fake piece of paper. Although now I see, my imagination was faulty from the beginning, that I imagined Alice was to get her money from a bank teller, an actual human being, who most possibly has a bullshit detector of their own, and would see through the attacker. I should've imagined an ATM, lifeless automaton. That would make my scenario more correct, and maybe I wouldn't miss this easy bug.
Next time, friends, next time.