code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardEscrow.sol#L106-L112 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardEscrow.sol#L207-L215

Vulnerability details

Impact

The user might pay more fees than expected when using MultiRewardEscrow.lock

Proof of Concept

The function MultiRewardEscrow.lock subtracts a fee from the toal amount used

uint256 feePerc = fees[token].feePerc;
if (feePerc > 0) {
    uint256 fee = Math.mulDiv(amount, feePerc, 1e18);

    amount -= fee;
    token.safeTransfer(feeRecipient, fee);
}

This fee is changeable with immediate effect by the owner:

  function setFees(IERC20[] memory tokens, uint256[] memory tokenFees) external onlyOwner {
    if (tokens.length != tokenFees.length) revert ArraysNotMatching(tokens.length, tokenFees.length);

    for (uint256 i = 0; i < tokens.length; i++) {
      if (tokenFees[i] >= 1e17) revert DontGetGreedy(tokenFees[i]);

      fees[tokens[i]].feePerc = tokenFees[i];
      emit FeeSet(tokens[i], tokenFees[i]);
    }
  }

This can lead to the user paying a higher fee than expected by the time the transaction executes if the owner changes the fees (no malicious intent required) at a similar time or the network gets congested.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider setting fees in a 2-step process as is done with other parts of the protocol, or pass another parameter (expected fees) to MultiRewardEscrow.lock to verify that fees are in line with what the user has seen at the time of sending the transaction.

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor acknowledged

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)