Lines of code
<https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/tokenomics/contracts/Tokenomics.sol#L1146&gt;
<https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/tokenomics/contracts/Treasury.sol#L402-L410&gt;

Vulnerability details

Impact

The withdrawToAccount() function of the Treasury contract is designed to send ETH rewards and OLAS top-ups to a specified account. However, there is a potential issue where a user’s reward transfer could silently fail. This occurs when the amountETHFromServices variable is less than accountRewards. In such a case, the function will still succeed if accountTopUps is greater than 0, but the user’s reward will not be transferred.

This function is only called through Dispenser.claimOwnerIncentives(), which first calls Tokenomics.accountOwnerIncentives() to obtain the reward and topUp amounts to pass on to Treasury.withdrawToAccount(). The accountOwnerIncentives() function always zeroes both values for the caller. The vulnerability arises because the reward is then zeroed even if the transfer silently fails in withdrawToAccount().

Since there is no limit on the amount of rewards a user may be claiming in one call, this can be considered loss of matured yield and is hence classified as high severity.

Proof of Concept

Consider a scenario where Alice is a user who is supposed to receive rewards. She call the Dispenser.claimOwnerIncentives() function, which calls Tokenomics.accountOwnerIncentives() to obtain Alice’s reward and top-up amounts and then calls Treasury.withdrawToAccount() with these amounts. If the amountETHFromServices is less than Alice’s reward amount, but her top-up amount is greater than 0, the withdrawToAccount function will still succeed. However, Alice’s reward will not be transferred, and her reward is zeroed in Tokenomics.accountOwnerIncentives(). As a result, Alice loses her expected reward.

Tools Used

Manual review

Recommended Mitigation Steps

Modify the withdrawToAccount function to revert or return false in the case where amountETHFromServices is less than accountRewards. A possible implementation may look as follows:

@@ -397,20 +397,25 @@ contract Treasury is IErrorsTokenomics {
             revert ManagerOnly(msg.sender, dispenser);
         }
 
+        success = true;
         uint256 amountETHFromServices = ETHFromServices;
         // Send ETH rewards, if any
-        if (accountRewards > 0 && amountETHFromServices >= accountRewards) {
-            amountETHFromServices -= accountRewards;
-            ETHFromServices = uint96(amountETHFromServices);
-            emit Withdraw(ETH_TOKEN_ADDRESS, account, accountRewards);
-            (success, ) = account.call{value: accountRewards}("");
-            if (!success) {
-                revert TransferFailed(address(0), address(this), account, accountRewards);
+        if (accountRewards > 0) {
+            if (amountETHFromServices >= accountRewards) {
+                amountETHFromServices -= accountRewards;
+                ETHFromServices = uint96(amountETHFromServices);
+                emit Withdraw(ETH_TOKEN_ADDRESS, account, accountRewards);
+                (success,) = account.call{value: accountRewards}("");
+                if (!success) {
+                    revert TransferFailed(address(0), address(this), account, accountRewards);
+                }
+            } else {
+                success = false;
             }
         }
 
         // Send OLAS top-ups
-        if (accountTopUps > 0) {
+        if (accountTopUps > 0 && success) {
             // Tokenomics has already accounted for the account's top-up amount,
             // thus the the mint does not break the inflation schedule
             IOLAS(olas).mint(account, accountTopUps);

Assessed type

ETH-Transfer

The text was updated successfully, but these errors were encountered:

All reactions