Description
Github username: --
Twitter username: --
Submission hash (on-chain): 0x15b630ae5bf5716a266e18fa85f7902cf84b81065b45b018c932b40bba938fa4
Severity: medium
Description:
Description
The AccumulatedFi contracts(inscope as well as out of scope) have used SafeTransferLib library which is take from Solmate as referenced here in Minter.sol.
The issue is with safeTransfer() and safeTransferFrom() functions which is extensively used across AccumulatedFi contracts and also in Minter.sol.
The used safeTransfer() and safeTransferFrom() function from solmate library doesn't check the existence of code at the token address. This is a known issue while using solmate's libraries.
As Per Natspec in Minter.sol which can be checked here
Note that none of the functions in this library check that a token has code at all! .....
Hence using safeTransferLib.sol library may lead to miscalculation of funds and may lead to loss of funds , because if safeTransferFrom() are called on a token address that doesn't have contract in it, it will always return success, bypassing the return value check. Due to this protocol will think that funds has been transferred to recipient address and the transaction would be successful, and records will be accordingly calculated, but in reality funds were never been transferred.
safeTransferFrom() and safeTranfer() function under the hood uses low level call function which can be checked here and here
call(gas(), token, 0, freeMemoryPointer, 68, 0, 32)However, solidity documentation strictly warns that,
The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.
code existence must be checked especially for low level functions like call,staticcall and delegatecall.
Openzeppelin and Solady confirms this and comply this requirements in their libraries, only currently used solmate does not check code existence.
Recommendation
Recommended to use openzeppelin's safeERC20 which takes care of token code existence OR alternatively check code existence as shown below:
function safeTransferFrom(
IERC20 token,
address from,
address to,
uint256 amount
) internal {
+ require(address(token).code.length != 0, "token does not exist");
bool success;
assembly {
// Get a pointer to some free memory.
let freeMemoryPointer := mload(0x40)
// Write the abi-encoded calldata into memory, beginning with the function selector.
mstore(freeMemoryPointer, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
mstore(add(freeMemoryPointer, 4), from) // Append the "from" argument.
mstore(add(freeMemoryPointer, 36), to) // Append the "to" argument.
mstore(add(freeMemoryPointer, 68), amount) // Append the "amount" argument.
success := and(
// Set success to whether the call reverted, if not we check it either
// returned exactly 1 (can't just be non-zero data), or had no return data.
or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
// We use 100 because the length of our calldata totals up like so: 4 + 32 * 3.
// We use 0 and 32 to copy up to 32 bytes of return data into the scratch space.
// Counterintuitively, this call must be positioned second to the or() call in the
// surrounding and() call or else returndatasize() will be zero during the computation.
call(gas(), token, 0, freeMemoryPointer, 100, 0, 32)
)
}
require(success, "TRANSFER_FROM_FAILED");
}
function safeTransfer(
IERC20 token,
address to,
uint256 amount
) internal {
+ require(address(token).code.length != 0, "token does not exist");
bool success;
assembly {
// Get a pointer to some free memory.
let freeMemoryPointer := mload(0x40)
// Write the abi-encoded calldata into memory, beginning with the function selector.
mstore(freeMemoryPointer, 0xa9059cbb00000000000000000000000000000000000000000000000000000000)
mstore(add(freeMemoryPointer, 4), to) // Append the "to" argument.
mstore(add(freeMemoryPointer, 36), amount) // Append the "amount" argument.
success := and(
// Set success to whether the call reverted, if not we check it either
// returned exactly 1 (can't just be non-zero data), or had no return data.
or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
// We use 68 because the length of our calldata totals up like so: 4 + 32 * 2.
// We use 0 and 32 to copy up to 32 bytes of return data into the scratch space.
// Counterintuitively, this call must be positioned second to the or() call in the
// surrounding and() call or else returndatasize() will be zero during the computation.
call(gas(), token, 0, freeMemoryPointer, 68, 0, 32)
)
}
require(success, "TRANSFER_FAILED");
}